Dynamic code modification support for x64 NaCl modules...

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=./tests/dynamic_code_loading/dynamic_modify_test.c

[petr, 2010-10-20 20:43:01]

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

http://codereview.chromium.org/3975001/diff/1/5#newcode486
src/trusted/validator_x86/ncvalidate_iter.c:486: if
(memcmp(&istate_old->num_prefix_bytes,
This may be a hack, let me know what you think

[bsy, 2010-10-20 21:18:28]

the hack is an ugly one.  please don't do that.

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

http://codereview.chromium.org/3975001/diff/1/5#newcode477
src/trusted/validator_x86/ncvalidate_iter.c:477: CHECK(416 ==
sizeof(NaClInstState));
why wouldn't these change from compiler to another?  why shouldn't future
changes add new members w/o looking at this code?  and when looking at this
code, what should future programmers do?  just mechanically change the number? 
why can't memcpy use sizeof?

http://codereview.chromium.org/3975001/diff/1/5#newcode479
src/trusted/validator_x86/ncvalidate_iter.c:479:
(uint8_t*)&istate_new->num_prefix_bytes));
ugh.  struct padding, which compilers are free to throw in, can change this
offset.

[Karl, 2010-10-21 17:38:32]

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

http://codereview.chromium.org/3975001/diff/1/3#newcode246
src/trusted/validator_x86/nccopycode.c:246: #if NACL_TARGET_SUBARCH == 64
Why are you not conditionalizing NCCopyCode (32-bit version) as well? We really
only want one model in the library's that are linked in. If you put both in, you
will bloat the minimum size of sel_ldr for 64-bit binaries.

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

http://codereview.chromium.org/3975001/diff/1/5#newcode479
src/trusted/validator_x86/ncvalidate_iter.c:479:
(uint8_t*)&istate_new->num_prefix_bytes));
On 2010/10/20 21:18:28, bsy wrote:
> ugh.  struct padding, which compilers are free to throw in, can change this
> offset.

I think it is safe to assume that they are the same iff the pointers are the
same. These pointers are looked up in a table. They should be "const", but
aren't. To make them const, I need to refactor the model generator, which has
not been done yet.

http://codereview.chromium.org/3975001/diff/1/5#newcode553
src/trusted/validator_x86/ncvalidate_iter.c:553: * Note that mstate_old was
validated when it was inserted originally.
This comment is a presumption about calling this routine. Therefore, it should
be in the header file so that user's need not look at the implementation!

[petr, 2010-10-21 19:36:28]

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

http://codereview.chromium.org/3975001/diff/1/3#newcode246
src/trusted/validator_x86/nccopycode.c:246: #if NACL_TARGET_SUBARCH == 64
On 2010/10/21 17:38:32, Karl wrote:
> Why are you not conditionalizing NCCopyCode (32-bit version) as well? We
really
> only want one model in the library's that are linked in. If you put both in,
you
> will bloat the minimum size of sel_ldr for 64-bit binaries.

Done.

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

http://codereview.chromium.org/3975001/diff/1/5#newcode477
src/trusted/validator_x86/ncvalidate_iter.c:477: CHECK(416 ==
sizeof(NaClInstState));
On 2010/10/20 21:18:28, bsy wrote:
> why wouldn't these change from compiler to another?  why shouldn't future
> changes add new members w/o looking at this code?  and when looking at this
> code, what should future programmers do?  just mechanically change the number?

> why can't memcpy use sizeof?

Done.

http://codereview.chromium.org/3975001/diff/1/5#newcode479
src/trusted/validator_x86/ncvalidate_iter.c:479:
(uint8_t*)&istate_new->num_prefix_bytes));
On 2010/10/20 21:18:28, bsy wrote:
> ugh.  struct padding, which compilers are free to throw in, can change this
> offset.

Done.

http://codereview.chromium.org/3975001/diff/1/5#newcode479
src/trusted/validator_x86/ncvalidate_iter.c:479:
(uint8_t*)&istate_new->num_prefix_bytes));
On 2010/10/20 21:18:28, bsy wrote:
> ugh.  struct padding, which compilers are free to throw in, can change this
> offset.

Done.

http://codereview.chromium.org/3975001/diff/1/5#newcode486
src/trusted/validator_x86/ncvalidate_iter.c:486: if
(memcmp(&istate_old->num_prefix_bytes,
On 2010/10/20 20:43:01, petr wrote:
> This may be a hack, let me know what you think

Done.

http://codereview.chromium.org/3975001/diff/1/5#newcode553
src/trusted/validator_x86/ncvalidate_iter.c:553: * Note that mstate_old was
validated when it was inserted originally.
On 2010/10/21 17:38:32, Karl wrote:
> This comment is a presumption about calling this routine. Therefore, it should
> be in the header file so that user's need not look at the implementation!

Done.

[Mark Seaborn, 2010-10-22 13:07:50]

On 2010/10/21 19:36:37, petr wrote:
> Denamic code modification support for x64 NaCl modules

"denamic" -> "dynamic"

[Karl, 2010-10-22 16:28:45]

LGTM.

Note: Before checking in, fix the description to fix the spelling of dynamic.

[bsy, 2010-10-23 02:29:53]

nits.  i prefer syscalls to never CHECK-crash the service runtime if at all
possible -- an error should be returned to the application.

http://codereview.chromium.org/3975001/diff/19001/11003
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/3975001/diff/19001/11003#newcode243
src/trusted/validator_x86/nccopycode.c:243: vstate = NCValidateInit(vbase,
vbase+sz, bundle_size);
are the globals g_SegFault -- called from ErrorSegmentation, called in
NCDecodeSegmentPair when replacement code is misaligned -- properly modified to
actually segfault/crash, or report an error?

this appears to be a standing bug/misfeature.  i'll file an issue on this.

http://codereview.chromium.org/3975001/diff/19001/11003#newcode267
src/trusted/validator_x86/nccopycode.c:267: CHECK(istate_old->length ==
istate_new->length &&
it would be nice if the new code and old code, if not properly in registration
wrt instruction lengths, not cause the service runtime to die w/ a CHECK failure
but to return an error for the modify code syscall.

[petr, 2010-10-25 17:01:23]

http://codereview.chromium.org/3975001/diff/19001/11003
File src/trusted/validator_x86/nccopycode.c (right):

http://codereview.chromium.org/3975001/diff/19001/11003#newcode267
src/trusted/validator_x86/nccopycode.c:267: CHECK(istate_old->length ==
istate_new->length &&
This condition is if-checked in NaClValidateInstReplacement, so at this point,
it should be true. However, if sel_ldr is invoked with -c than we may have this
CHECK fail here. I am gonna do if-check here.


On 2010/10/23 02:29:53, bsy wrote:
> it would be nice if the new code and old code, if not properly in registration
> wrt instruction lengths, not cause the service runtime to die w/ a CHECK
failure
> but to return an error for the modify code syscall.

[petr, 2010-10-27 19:52:42]

Bennet: plase, take another look

[bsy, 2010-11-01 23:50:50]

this code looks pretty good, but we should have tests to prevent regressions. 
please add tests to exercise this code, and amend the CL description with TEST=
and BUG= lines.

http://codereview.chromium.org/3975001/diff/26001/27004
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/3975001/diff/26001/27004#newcode554
src/trusted/validator_x86/ncvalidate_iter.c:554: 
it's not your API, but it would be useful if there were a comment somewhere in
this file or in the header file reminding that LOG_ERROR is how we ... thorough
a long convoluted sequence... set that there was a validator error (validates_ok
to false) and the quit flag.

[petr, 2010-11-05 00:13:50]

Bennet, can you have another look please.

dynamic_load_test.c runs as a small tests and inside browser, do we want to make
dynamic_modify_test.c the same? Currently, dynamic_modify_test.c is a small test
only.

http://codereview.chromium.org/3975001/diff/26001/27004
File src/trusted/validator_x86/ncvalidate_iter.c (right):

http://codereview.chromium.org/3975001/diff/26001/27004#newcode554
src/trusted/validator_x86/ncvalidate_iter.c:554: 
added comment at the beginning of this function, and added comment in .h file to
NaClValidateSegmentPair.

On 2010/11/01 23:50:51, bsy wrote:
> it's not your API, but it would be useful if there were a comment somewhere in
> this file or in the header file reminding that LOG_ERROR is how we ...
thorough
> a long convoluted sequence... set that there was a validator error
(validates_ok
> to false) and the quit flag.

[Mark Seaborn, 2010-11-10 10:12:10]

On 2010/11/05 00:13:50, petr wrote:
> dynamic_load_test.c runs as a small tests and inside browser, do we want
> to make dynamic_modify_test.c the same? Currently, dynamic_modify_test.c
> is a small test only.

Yes, please do add it to tests/inbrowser_test_runner.  Self-contained, non-crash
tests should be easy to add there.

Cheers,
Mark

[Mark Seaborn, 2010-11-10 11:03:42]

bsy wrote:
> i have no idea why dynamic load test needs to run inside the
> browser; that seems to make the test rely on a lot more machinery
> that it could do without.  mseaborn: rationale?

It can run both inside and outside the browser.  We need to be able to
run it inside the browser in order to test that machinery:  Until my
Mac changes are committed, dynamic loading doesn't work in the Chrome
Mac sandbox, and we need a way of testing that.

Mark

[petr, 2010-11-11 07:42:59]

enable dynalic_modify_test to run in a browser

Bennet, Mark, please have another look

[bsy, 2010-11-11 21:27:21]

mostly nits.

kschimpf: plz comment on ncvalidate_iter.c

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

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:269: NaClLog(LOG_INFO,
if i understand what you said earlier, this should never occur if the validation
phase passed.  given that, i think this should be a higher severity level than
LOG_INFO -- a LOG_ERROR (or even a LOG_FATAL) would be appropriate.  (yeah, i
know, i asked for an error return earlier.)

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

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:244: /* TODO(karl): empty fmt
strings not allowed */
is this TODO still valid, given the check below?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:249: if (format != NULL) {
when do we get NULL format strings?  this sounds like an internal error in the
validator.  kschimpf: opinions?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:571:
NaClValidatorInstMessage(LOG_ERROR, state, istate_old, NULL);
are these two lines the only places where the format string might be NULL?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/ncvalidate_iter.h (right):

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.h:198: /* Validate a segment for
dynamic code replacement
/*
 * comment text starts on this line, not above.
 * more comments.
 */

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
File tests/dynamic_code_loading/dynamic_load_test.c (right):

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:21: #define BUF_SIZE 64
why should code buffers be 64 bytes on x86-64 cpus?  a comment please.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:102: assert(dest_size % 32 == 0);
is this 32 (always) correct?  should it be 16 or 32, depending on bundle size?

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:172: uint8_t buf[32];
should this be BUF_SIZE?  if not, a comment would be nice.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:207: uint8_t nops[32];
should this be BUF_SIZE too?  if not, a comment would be nice.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:212: rc =
nacl_load_code(load_area + 1, nops, 32);
NACL_ARRAY_SIZE(nops) or sizeof nops instead of 32?

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:214: rc =
nacl_load_code(load_area + 4, nops, 32);
ditto

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:218: rc =
nacl_load_code(load_area, nops + 1, 31);
NACL_ARRAY_SIZE(nops) - 1 instead of 31?

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:220: rc =
nacl_load_code(load_area, nops + 4, 28);
similar to above

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:224: rc =
nacl_load_code(load_area, nops, 32);
!!

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:249: rc = nacl_load_code(data,
data, 32);
!!

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:307: uint8_t data[32];
!!

[Karl, 2010-11-11 22:45:41]

Minor nit, agreeing with Bennet. Consider making a separate message function,
with 2 instruction arguments, rather than making "null" format strings a special
case.

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

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:571:
NaClValidatorInstMessage(LOG_ERROR, state, istate_old, NULL);
On 2010/11/11 21:27:21, bsy wrote:
> are these two lines the only places where the format string might be NULL?

I think that you may just want to add a new "message" function
NaClValidatorInstsMessage(....) that gets two instructions, and prints them with
a message.

[petr, 2010-11-12 18:11:06]

please have another look

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

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/nccopycode.c:269: NaClLog(LOG_INFO,
This can occur if you run sel_ldr with -c. We should not change this to
LOG_FATAL as then the execution will abort at this point but LOG_ERROR looks OK.

On 2010/11/11 21:27:21, bsy wrote:
> if i understand what you said earlier, this should never occur if the
validation
> phase passed.  given that, i think this should be a higher severity level than
> LOG_INFO -- a LOG_ERROR (or even a LOG_FATAL) would be appropriate.  (yeah, i
> know, i asked for an error return earlier.)

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

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:244: /* TODO(karl): empty fmt
strings not allowed */
I rolled back the check below, so it is valid 
On 2010/11/11 21:27:21, bsy wrote:
> is this TODO still valid, given the check below?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:249: if (format != NULL) {
I use this to print two instructions in a row, see the uses below. So, my uses
are the only that give format = NULL.
I will adopt Karl's suggestion and create a separate message function

On 2010/11/11 21:27:21, bsy wrote:
> when do we get NULL format strings?  this sounds like an internal error in the
> validator.  kschimpf: opinions?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:571:
NaClValidatorInstMessage(LOG_ERROR, state, istate_old, NULL);
Yes, they are
On 2010/11/11 21:27:21, bsy wrote:
> are these two lines the only places where the format string might be NULL?

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.c:571:
NaClValidatorInstMessage(LOG_ERROR, state, istate_old, NULL);
Yes, I will do this

On 2010/11/11 22:45:41, Karl wrote:
> On 2010/11/11 21:27:21, bsy wrote:
> > are these two lines the only places where the format string might be NULL?
> 
> I think that you may just want to add a new "message" function
> NaClValidatorInstsMessage(....) that gets two instructions, and prints them
with
> a message.

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
File src/trusted/validator_x86/ncvalidate_iter.h (right):

http://codereview.chromium.org/3975001/diff/93001/src/trusted/validator_x86/n...
src/trusted/validator_x86/ncvalidate_iter.h:198: /* Validate a segment for
dynamic code replacement
On 2010/11/11 21:27:21, bsy wrote:
> /*
>  * comment text starts on this line, not above.
>  * more comments.
>  */

Done.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
File tests/dynamic_code_loading/dynamic_load_test.c (right):

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:21: #define BUF_SIZE 64
on X86-64 we operate with code samples that do not fit in 32-byte buffers
On 2010/11/11 21:27:21, bsy wrote:
> why should code buffers be 64 bytes on x86-64 cpus?  a comment please.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:102: assert(dest_size % 32 == 0);
On 2010/11/11 21:27:21, bsy wrote:
> is this 32 (always) correct?  should it be 16 or 32, depending on bundle size?

Done.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:172: uint8_t buf[32];
On 2010/11/11 21:27:21, bsy wrote:
> should this be BUF_SIZE?  if not, a comment would be nice.

Done.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:207: uint8_t nops[32];
On 2010/11/11 21:27:21, bsy wrote:
> should this be BUF_SIZE too?  if not, a comment would be nice.

Done.

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:212: rc =
nacl_load_code(load_area + 1, nops, 32);
we are testing that the size of a segment is bundle aligned so

s/32/NACL_BUNDLE_SIZE/
On 2010/11/11 21:27:21, bsy wrote:
> NACL_ARRAY_SIZE(nops) or sizeof nops instead of 32?

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:214: rc =
nacl_load_code(load_area + 4, nops, 32);
ditto
On 2010/11/11 21:27:21, bsy wrote:
> ditto

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:218: rc =
nacl_load_code(load_area, nops + 1, 31);
ditto
On 2010/11/11 21:27:21, bsy wrote:
> NACL_ARRAY_SIZE(nops) - 1 instead of 31?

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:220: rc =
nacl_load_code(load_area, nops + 4, 28);
ditto
On 2010/11/11 21:27:21, bsy wrote:
> similar to above

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:224: rc =
nacl_load_code(load_area, nops, 32);
probably, it is just an additional check (maybe redundant)
s/32/NACL_BUNDLE_SIZE/ 
On 2010/11/11 21:27:21, bsy wrote:
> !!

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:249: rc = nacl_load_code(data,
data, 32);
s/32/NACL_BUNDLE_SIZE/
On 2010/11/11 21:27:21, bsy wrote:
> !!

http://codereview.chromium.org/3975001/diff/93001/tests/dynamic_code_loading/...
tests/dynamic_code_loading/dynamic_load_test.c:307: uint8_t data[32];
On 2010/11/11 21:27:21, bsy wrote:
> !!

Done.