Resurrect Petr's 64-bit dynamic code modification CL:...

Resurrect Petr's 64-bit dynamic code modification CL:
http://codereview.chromium.org/3975001/

previous CL description:

Dynamic code modification support for x64 NaCl modules 
1) enables validation of modified code 
2) enables thread-safe copying of code modification 
3) added tests to exercise dynamic code modification 


BUG= none
TEST= run_dynamic_load_test


Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=4100

[elijahtaylor (use chromium), 2010-12-10 20:01:40]

This first rev is the direct patch from http://codereview.chromium.org/3975001/,
I'm taking it over to get it checked in.

Passing trybots minus one possibly flaky bot, will investigate.

I've added a couple nits plus a couple of substantial questions.  PTAL

http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
src/trusted/service_runtime/sel_validate_image.c:142: return LOAD_BAD_FILE;
does 'vstate' need to be cleaned up here? or move this check above vstate
creation?

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
src/trusted/validator_x86/nccopycode.c:191: /* we assume a 1-byte change it
atomic */
s/it/is/

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
src/trusted/validator_x86/ncvalidate_iter.c:554: break;
are these breaks in this for loop correct?  It seems to me like they're meant
for the do/while loop to indicate an error, but these will only break from the
for loop and return, indicating no error, right?

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
src/trusted/validator_x86/ncvalidate_iter.c:560: * allow modification of
sandboxing instructions. Note nether of the
s/nether/neither/

[Karl, 2010-12-10 21:18:15]

I will look more at the changes later, but the points you bring out are all good
points and should be changed.

http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
src/trusted/service_runtime/sel_validate_image.c:142: return LOAD_BAD_FILE;
On 2010/12/10 20:01:40, elijahtaylor wrote:
> does 'vstate' need to be cleaned up here? or move this check above vstate
> creation?

 yes you need to do one of the above. Otherwise memory will leak.

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
src/trusted/validator_x86/nccopycode.c:191: /* we assume a 1-byte change it
atomic */
On 2010/12/10 20:01:40, elijahtaylor wrote:
> s/it/is/

Yes this should be changed as you suggest.

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
src/trusted/validator_x86/ncvalidate_iter.c:554: break;
On 2010/12/10 20:01:40, elijahtaylor wrote:
> are these breaks in this for loop correct?  It seems to me like they're meant
> for the do/while loop to indicate an error, but these will only break from the
> for loop and return, indicating no error, right?

Good point.  if they are different, an error should bew reported.

http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
src/trusted/validator_x86/ncvalidate_iter.c:560: * allow modification of
sandboxing instructions. Note nether of the
On 2010/12/10 20:01:40, elijahtaylor wrote:
> s/nether/neither/

This correction should be made.

[elijahtaylor (use chromium), 2010-12-11 01:30:02]

Addressed all of these issues.

On 2010/12/10 21:18:15, Karl wrote:
> I will look more at the changes later, but the points you bring out are all
good
> points and should be changed.
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/service_runtime/sel...
> src/trusted/service_runtime/sel_validate_image.c:142: return LOAD_BAD_FILE;
> On 2010/12/10 20:01:40, elijahtaylor wrote:
> > does 'vstate' need to be cleaned up here? or move this check above vstate
> > creation?
> 
>  yes you need to do one of the above. Otherwise memory will leak.
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
> File src/trusted/validator_x86/nccopycode.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/nccop...
> src/trusted/validator_x86/nccopycode.c:191: /* we assume a 1-byte change it
> atomic */
> On 2010/12/10 20:01:40, elijahtaylor wrote:
> > s/it/is/
> 
> Yes this should be changed as you suggest.
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
> File src/trusted/validator_x86/ncvalidate_iter.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
> src/trusted/validator_x86/ncvalidate_iter.c:554: break;
> On 2010/12/10 20:01:40, elijahtaylor wrote:
> > are these breaks in this for loop correct?  It seems to me like they're
meant
> > for the do/while loop to indicate an error, but these will only break from
the
> > for loop and return, indicating no error, right?
> 
> Good point.  if they are different, an error should bew reported.
> 
>
http://codereview.chromium.org/5738003/diff/1/src/trusted/validator_x86/ncval...
> src/trusted/validator_x86/ncvalidate_iter.c:560: * allow modification of
> sandboxing instructions. Note nether of the
> On 2010/12/10 20:01:40, elijahtaylor wrote:
> > s/nether/neither/
> 
> This correction should be made.

[Karl, 2010-12-13 20:38:42]

The validator/sel_loader look good to me. However, I do not know enough about
what the template files are doing to approve this CL. If Bennet or Mark accept
those parts, I'm willing to approve this.

[bsy, 2010-12-15 04:06:50]

some feedback.

most important:  please add a test that would have exposed the double break
security bug.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:62: 
same CHECK as below would be good (yeah, i know, pre-existing code).

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:137: 
a check here that guest_addr starts at bundle boundary and size is a multiple of
bundle size would be good.  this is a necessary precondition which is
(currently) satisfied, but we shouldn't let the code drift/rot.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:265: istate_old =
NaClInstIterGetState(iter_old);
does this get pseudo-atomic sequences as a single NaClnstState?  comment on
assumptions made by this code... e.g., this is only used after
NaClValidateCodeReplacement

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:270: "Segment replacement: copied
instructions misaligned\n");
is this precondition violation that should never happen, due to
NaClValidateCodeReplacement?  if so, a comment and a LOG_FATAL would be more
appropriate; if not, then NaClInstIterDestroys are necessary to properly clean
up.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:273:
CopyInstructionInternal(istate_old->mpc,
add a comment here that replacing all affected instructions' first bytes with
HLT, then a SerializeAllProcessors, then the rest of the bytes with the desired
replacement values, another SerializeAllProcessors, then replace the first byte
of each instruction with the desired values would save overhead, rather than
doing 2 calls to SerializeAllProcessors per instruction.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:290: if (state->do_stub_out &&
(level <= LOG_ERROR)) {
this appears to be dead code, since do_stub_out must be false in order to not
early exit in NaClValidateCodeReplacement, which so far is the only way to get
to NaClValidatorTwoInstMessage.  We should probably replace this with a comment
that we don't stub out when it's a two inst streams.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:556: error_in_nodes = TRUE;
consider using a goto for the double break here and below, since that's clearer
what the intent was.  (it would have avoided the bug that you found!)

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:596: if (i -
NaClGetExpParentIndex(exp_old, i) == 4)
magic number -- do we need/want a kDisplacementNode or something?  the e.g.
above might be good enough.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:604: if (error_in_nodes)
good catch!  NaClValidatorTwoInstMessage -> NaClRecordIfValidatorError is
required to set valdates_ok to FALSE.  since this style of using a logging
function to record critical info is somewhat non-obvious, a comment here (or
elsewhere in this file) would be good.

for extra extra goodness, ensure we have a test case that this check catches,
but wouldn't otherwise.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:614: void
NaClValidateSegmentPair(uint8_t* mbase_old, uint8_t* mbase_new,
a comment here about assumptions -- that vbase/size are 0
mod bundle_size -- would be good.  o/w the buffer might end in the middle of an
atomic code sequence, and the pairwise validation might not catch that the last
instruction is a mask instruction that cannot be changed.

[elijahtaylor (use chromium), 2010-12-15 21:26:37]

Feedback addressed (one push back and one not done because of questions)

I will craft a test to exercise the error checking in the previous "double
break" sections, but it's not included in this upload.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:62: 
Done

On 2010/12/15 04:06:50, bsy wrote:
> same CHECK as below would be good (yeah, i know, pre-existing code).

http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:137: 
Done.

On 2010/12/15 04:06:50, bsy wrote:
> a check here that guest_addr starts at bundle boundary and size is a multiple
of
> bundle size would be good.  this is a necessary precondition which is
> (currently) satisfied, but we shouldn't let the code drift/rot.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:265: istate_old =
NaClInstIterGetState(iter_old);
Comment added to clarify what we're doing.

On 2010/12/15 04:06:50, bsy wrote:
> does this get pseudo-atomic sequences as a single NaClnstState?  comment on
> assumptions made by this code... e.g., this is only used after
> NaClValidateCodeReplacement

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:270: "Segment replacement: copied
instructions misaligned\n");
Changed to LOG_FATAL and commented as this should be guaranteed not to happen.

On 2010/12/15 04:06:50, bsy wrote:
> is this precondition violation that should never happen, due to
> NaClValidateCodeReplacement?  if so, a comment and a LOG_FATAL would be more
> appropriate; if not, then NaClInstIterDestroys are necessary to properly clean
> up.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:273:
CopyInstructionInternal(istate_old->mpc,
Done.

On 2010/12/15 04:06:50, bsy wrote:
> add a comment here that replacing all affected instructions' first bytes with
> HLT, then a SerializeAllProcessors, then the rest of the bytes with the
desired
> replacement values, another SerializeAllProcessors, then replace the first
byte
> of each instruction with the desired values would save overhead, rather than
> doing 2 calls to SerializeAllProcessors per instruction.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:290: if (state->do_stub_out &&
(level <= LOG_ERROR)) {
I'm very unfamiliar with this, so I haven't done this yet... what is do_stub_out
anyway?

On 2010/12/15 04:06:50, bsy wrote:
> this appears to be dead code, since do_stub_out must be false in order to not
> early exit in NaClValidateCodeReplacement, which so far is the only way to get
> to NaClValidatorTwoInstMessage.  We should probably replace this with a
comment
> that we don't stub out when it's a two inst streams.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:556: error_in_nodes = TRUE;
I am loath to "goto", but I agree that's clearer in this case. Done.

On 2010/12/15 04:06:50, bsy wrote:
> consider using a goto for the double break here and below, since that's
clearer
> what the intent was.  (it would have avoided the bug that you found!)

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:596: if (i -
NaClGetExpParentIndex(exp_old, i) == 4)
Not done.  I consider the comment good enough to explain this, particularly if
we don't have a full set of constants that explain every node index type.

On 2010/12/15 04:06:50, bsy wrote:
> magic number -- do we need/want a kDisplacementNode or something?  the e.g.
> above might be good enough.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:604: if (error_in_nodes)
Commented both locations (valid return and error return)

On 2010/12/15 04:06:50, bsy wrote:
> good catch!  NaClValidatorTwoInstMessage -> NaClRecordIfValidatorError is
> required to set valdates_ok to FALSE.  since this style of using a logging
> function to record critical info is somewhat non-obvious, a comment here (or
> elsewhere in this file) would be good.
> 
> for extra extra goodness, ensure we have a test case that this check catches,
> but wouldn't otherwise.

http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:614: void
NaClValidateSegmentPair(uint8_t* mbase_old, uint8_t* mbase_new,
Done.

On 2010/12/15 04:06:50, bsy wrote:
> a comment here about assumptions -- that vbase/size are 0
> mod bundle_size -- would be good.  o/w the buffer might end in the middle of
an
> atomic code sequence, and the pairwise validation might not catch that the
last
> instruction is a mask instruction that cannot be changed.

[elijahtaylor (use chromium), 2010-12-15 22:24:40]

Added test to exercise the 'goto' code.  I didn't test all cases because I
wasn't sure if it's even possible to have an instruction with different
kind/flags nodes that had so much in common per previous checks.

Admittedly the check I added would probably be an ok replacement, but it's
currently disallowed, so I used it :)


On 2010/12/15 21:26:37, elijahtaylor wrote:
> Feedback addressed (one push back and one not done because of questions)
> 
> I will craft a test to exercise the error checking in the previous "double
> break" sections, but it's not included in this upload.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
> src/trusted/service_runtime/sel_validate_image.c:62: 
> Done
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > same CHECK as below would be good (yeah, i know, pre-existing code).
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/service_runtime...
> src/trusted/service_runtime/sel_validate_image.c:137: 
> Done.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > a check here that guest_addr starts at bundle boundary and size is a
multiple
> of
> > bundle size would be good.  this is a necessary precondition which is
> > (currently) satisfied, but we shouldn't let the code drift/rot.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> File src/trusted/validator_x86/nccopycode.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/nccopycode.c:265: istate_old =
> NaClInstIterGetState(iter_old);
> Comment added to clarify what we're doing.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > does this get pseudo-atomic sequences as a single NaClnstState?  comment on
> > assumptions made by this code... e.g., this is only used after
> > NaClValidateCodeReplacement
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/nccopycode.c:270: "Segment replacement: copied
> instructions misaligned\n");
> Changed to LOG_FATAL and commented as this should be guaranteed not to happen.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > is this precondition violation that should never happen, due to
> > NaClValidateCodeReplacement?  if so, a comment and a LOG_FATAL would be more
> > appropriate; if not, then NaClInstIterDestroys are necessary to properly
clean
> > up.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/nccopycode.c:273:
> CopyInstructionInternal(istate_old->mpc,
> Done.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > add a comment here that replacing all affected instructions' first bytes
with
> > HLT, then a SerializeAllProcessors, then the rest of the bytes with the
> desired
> > replacement values, another SerializeAllProcessors, then replace the first
> byte
> > of each instruction with the desired values would save overhead, rather than
> > doing 2 calls to SerializeAllProcessors per instruction.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> File src/trusted/validator_x86/ncvalidate_iter.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/ncvalidate_iter.c:290: if (state->do_stub_out &&
> (level <= LOG_ERROR)) {
> I'm very unfamiliar with this, so I haven't done this yet... what is
do_stub_out
> anyway?
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > this appears to be dead code, since do_stub_out must be false in order to
not
> > early exit in NaClValidateCodeReplacement, which so far is the only way to
get
> > to NaClValidatorTwoInstMessage.  We should probably replace this with a
> comment
> > that we don't stub out when it's a two inst streams.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/ncvalidate_iter.c:556: error_in_nodes = TRUE;
> I am loath to "goto", but I agree that's clearer in this case. Done.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > consider using a goto for the double break here and below, since that's
> clearer
> > what the intent was.  (it would have avoided the bug that you found!)
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/ncvalidate_iter.c:596: if (i -
> NaClGetExpParentIndex(exp_old, i) == 4)
> Not done.  I consider the comment good enough to explain this, particularly if
> we don't have a full set of constants that explain every node index type.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > magic number -- do we need/want a kDisplacementNode or something?  the e.g.
> > above might be good enough.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/ncvalidate_iter.c:604: if (error_in_nodes)
> Commented both locations (valid return and error return)
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > good catch!  NaClValidatorTwoInstMessage -> NaClRecordIfValidatorError is
> > required to set valdates_ok to FALSE.  since this style of using a logging
> > function to record critical info is somewhat non-obvious, a comment here (or
> > elsewhere in this file) would be good.
> > 
> > for extra extra goodness, ensure we have a test case that this check
catches,
> > but wouldn't otherwise.
> 
>
http://codereview.chromium.org/5738003/diff/11001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/ncvalidate_iter.c:614: void
> NaClValidateSegmentPair(uint8_t* mbase_old, uint8_t* mbase_new,
> Done.
> 
> On 2010/12/15 04:06:50, bsy wrote:
> > a comment here about assumptions -- that vbase/size are 0
> > mod bundle_size -- would be good.  o/w the buffer might end in the middle of
> an
> > atomic code sequence, and the pairwise validation might not catch that the
> last
> > instruction is a mask instruction that cannot be changed.

[bsy, 2011-01-05 00:56:22]

a few style/comment nits.  o/w lgtm.  thanx for being on top of this!

http://codereview.chromium.org/5738003/diff/32001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/5738003/diff/32001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:154: if (vstate == NULL) {
plz use NULL == vstate style in TCB code.

http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/nccopycode.h (right):

http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.h:13: /* copies code from src to dest in a
thread safe way, returns 0 on success */
what do non-zero values mean?

http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.h:18: int NaClCopyCodeIter(uint8_t *dst,
uint8_t *src,
plz add comment on return value convention.  this seems a little weird, in that
0 is success and 1 is failure.  in the rest of the TCB, we have bool style
functions and syscall style functions, where the former style used 0/1 for
failure/success, and the latter style used negated errno values for (a range of)
failures and others indicate success.

consider switching to one of these, probably the bool style.

http://codereview.chromium.org/5738003/diff/32001/tests/dynamic_code_loading/...
File tests/dynamic_code_loading/dynamic_modify_test.c (right):

http://codereview.chromium.org/5738003/diff/32001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_modify_test.c:156: assert(first_diff>0 &&
first_diff<=sizeof(buf));
spaces around <= and >

[elijahtaylor (use chromium), 2011-01-10 19:27:05]

Just a heads up: I'm committing this today once my latest try comes back green
if I don't get any objections.  I fixed the nits from the last round, but also
fixed a couple other test-related things due to redness of the trybots.


On 2011/01/05 00:56:22, bsy wrote:
> a few style/comment nits.  o/w lgtm.  thanx for being on top of this!
> 
>
http://codereview.chromium.org/5738003/diff/32001/src/trusted/service_runtime...
> File src/trusted/service_runtime/sel_validate_image.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/32001/src/trusted/service_runtime...
> src/trusted/service_runtime/sel_validate_image.c:154: if (vstate == NULL) {
> plz use NULL == vstate style in TCB code.
> 
>
http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
> File src/trusted/validator_x86/nccopycode.h (right):
> 
>
http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/nccopycode.h:13: /* copies code from src to dest in
a
> thread safe way, returns 0 on success */
> what do non-zero values mean?
> 
>
http://codereview.chromium.org/5738003/diff/32001/src/trusted/validator_x86/n...
> src/trusted/validator_x86/nccopycode.h:18: int NaClCopyCodeIter(uint8_t *dst,
> uint8_t *src,
> plz add comment on return value convention.  this seems a little weird, in
that
> 0 is success and 1 is failure.  in the rest of the TCB, we have bool style
> functions and syscall style functions, where the former style used 0/1 for
> failure/success, and the latter style used negated errno values for (a range
of)
> failures and others indicate success.
> 
> consider switching to one of these, probably the bool style.
> 
>
http://codereview.chromium.org/5738003/diff/32001/tests/dynamic_code_loading/...
> File tests/dynamic_code_loading/dynamic_modify_test.c (right):
> 
>
http://codereview.chromium.org/5738003/diff/32001/tests/dynamic_code_loading/...
> tests/dynamic_code_loading/dynamic_modify_test.c:156: assert(first_diff>0 &&
> first_diff<=sizeof(buf));
> spaces around <= and >