Don't modify memory access right when changing data in debug stub.

Don't modify memory access rights when changing data in debug stub.
Add test for calling program functions from gdb.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2990
TEST= run_gdb_call_test
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=9622

[eaeltsin, 2012-08-29 16:12:17]

Am I right that IPlatform::SetMemory can be also used to modify user data? For
example check the following GDB command:
  (gdb) set variable <your-variable-name>=<new-value>

Please check this is supported. Please add a test for that.

[halyavin, 2012-08-29 17:03:43]

Yes. I plan to add this test in the next CL.

Andrey Khalyavin

2012/8/29 <eaeltsin@google.com>

> Am I right that IPlatform::SetMemory can be also used to modify user data?
> For
> example check the following GDB command:
>   (gdb) set variable <your-variable-name>=<new-**value>
>
> Please check this is supported. Please add a test for that.
>
>
>
https://chromiumcodereview.**appspot.com/10896004/<https://chromiumcodereview...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[eaeltsin, 2012-08-29 17:13:09]

On 2012/08/29 17:03:43, halyavin wrote:
> Yes. I plan to add this test in the next CL.

I would definitely prefer if you add it to this CL.

> 
> Andrey Khalyavin
> 
> 2012/8/29 <mailto:eaeltsin@google.com>
> 
> > Am I right that IPlatform::SetMemory can be also used to modify user data?
> > For
> > example check the following GDB command:
> >   (gdb) set variable <your-variable-name>=<new-**value>
> >
> > Please check this is supported. Please add a test for that.
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/10896004/%3Chttps://chromiumcoderevi...>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> To unsubscribe from this group, send email to
> mailto:native-client-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/native-client-reviews?hl=en.
>

[Mark Seaborn, 2012-08-29 17:29:43]

"Don't" would be the usual tense to use in the commit message rather than
"Doesn't".

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
File src/trusted/debug_stub/platform.h (right):

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
src/trusted/debug_stub/platform.h:34: static bool SetMemory(struct NaClApp* nap,
uint64_t address, uint32_t length,
Use the " *" spacing style, please

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
src/trusted/debug_stub/posix/platform_impl.cc:70: bool
IPlatform::SetMemory(struct NaClApp* nap, uint64_t virt, uint32_t len,
Use the " *" spacing style

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
src/trusted/debug_stub/posix/platform_impl.cc:76: if (is_code &&
mprotect(reinterpret_cast<void*>(page), mapping_size,
Nit: I would suggest this is clearer as a nested 'if':
if (is_code) {
  if (mprotect(...) != 0) {
    handle error...
  }
}
to separate the side-effecting, error-checked call from the condition.

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
src/trusted/debug_stub/posix/platform_impl.cc:77: PROT_READ | PROT_WRITE) != 0)
{
Should we change this to read+write+exec so as not to interfere with
concurrently-running code?  (On the other hand, I think all threads are
suspended when this is called.)

https://chromiumcodereview.appspot.com/10896004/diff/10004/src/trusted/debug_...
src/trusted/debug_stub/posix/platform_impl.cc:81: // TODO(mseaborn): We use
mprotect only to modify code area, so
You have left a 'TODO' here but the comment does not seem to describe a task to
do.

Maybe change to:

"We use mprotect() only to modify the code area, so PROT_READ | PROT_EXEC are
the correct flags to restore the mapping to in most cases.  However, this does
not behave correctly for zero page (which we will make readable) or
as-yet-unallocated pages in the dynamic code area (where we will make zeroed
bytes executable).

TODO(mseaborn): Handle those cases correctly.  We might do that by modifying
code via the dynamic code area the same way nacl_text.c does."

[halyavin, 2012-08-30 09:46:28]

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pla...
File src/trusted/debug_stub/platform.h (right):

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pla...
src/trusted/debug_stub/platform.h:34: static bool SetMemory(struct NaClApp* nap,
uint64_t address, uint32_t length,
On 2012/08/29 17:29:43, Mark Seaborn wrote:
> Use the " *" spacing style, please

Done.

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pos...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:70: bool
IPlatform::SetMemory(struct NaClApp* nap, uint64_t virt, uint32_t len,
On 2012/08/29 17:29:43, Mark Seaborn wrote:
> Use the " *" spacing style

Done.

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:76: if (is_code &&
mprotect(reinterpret_cast<void*>(page), mapping_size,
On 2012/08/29 17:29:43, Mark Seaborn wrote:
> Nit: I would suggest this is clearer as a nested 'if':
> if (is_code) {
>   if (mprotect(...) != 0) {
>     handle error...
>   }
> }
> to separate the side-effecting, error-checked call from the condition.

Done.

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:77: PROT_READ | PROT_WRITE) != 0)
{
On 2012/08/29 17:29:43, Mark Seaborn wrote:
> Should we change this to read+write+exec so as not to interfere with
> concurrently-running code?  (On the other hand, I think all threads are
> suspended when this is called.)
Yes, we are not going to support non-stop mode any time soon. And so now we
should follow least rights policy to catch bugs.

http://codereview.chromium.org/10896004/diff/10004/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:81: // TODO(mseaborn): We use
mprotect only to modify code area, so
On 2012/08/29 17:29:43, Mark Seaborn wrote:
> You have left a 'TODO' here but the comment does not seem to describe a task
to
> do.
Changed wording a bit to clarify why zero page is different.

[Mark Seaborn, 2012-08-30 16:55:57]

Sorry to be picky -- I looked at the test more closely and realised it could be
simpler.

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/pos...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:79: return false;
Nit: Indentation is not a multiple of 2 here.  Indent by 1 space less.

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/win...
File src/trusted/debug_stub/win/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/win...
src/trusted/debug_stub/win/platform_impl.cc:51: bool IPlatform::SetMemory(struct
NaClApp* nap, uint64_t virt, uint32_t len,
Use the " *" spacing style here

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/call.py
File tests/gdb/call.py (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/call.py#newcode11
tests/gdb/call.py:11: gdb.Command('break test_call')
Why not just set a breakpoint on main()?  Then you don't need to delete the
breakpoint, and you don't need the C code to call test_call()?

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/gdb_test_guest.c
File tests/gdb/gdb_test_guest.c (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/gdb_test_guest.c...
tests/gdb/gdb_test_guest.c:58: global_var += arg;
This doesn't really need global_var to be involved.  You could return arg*2 and
check that test_call(123)==468 (larger numbers are easier to grep for).

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/gdb_test_guest.c...
tests/gdb/gdb_test_guest.c:85: if (strcmp(argv[1], "call") == 0) {
If you set a breakpoint on main(), you don't need this.

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/nacl.scons
File tests/gdb/nacl.scons (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/nacl.scons#newco...
tests/gdb/nacl.scons:73: command=['${PYTHON}', env.File('call.py')],
Nit: "call" is a bit generic.  Can you call this something like "call_from_gdb"
or "manufactured_call"?  Something to indicate that GDB is doing something a
little scary when it hijacks a thread and modifies its registers to run a
function of its choosing. :-)

[halyavin, 2012-08-31 08:01:24]

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/pos...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:79: return false;
On 2012/08/30 16:55:57, Mark Seaborn wrote:
> Nit: Indentation is not a multiple of 2 here.  Indent by 1 space less.

Done.

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/win...
File src/trusted/debug_stub/win/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/15001/src/trusted/debug_stub/win...
src/trusted/debug_stub/win/platform_impl.cc:51: bool IPlatform::SetMemory(struct
NaClApp* nap, uint64_t virt, uint32_t len,
On 2012/08/30 16:55:57, Mark Seaborn wrote:
> Use the " *" spacing style here

Done.

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/call.py
File tests/gdb/call.py (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/call.py#newcode11
tests/gdb/call.py:11: gdb.Command('break test_call')
On 2012/08/30 16:55:57, Mark Seaborn wrote:
> Why not just set a breakpoint on main()?  Then you don't need to delete the
> breakpoint, and you don't need the C code to call test_call()?

Done.

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/gdb_test_guest.c
File tests/gdb/gdb_test_guest.c (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/gdb_test_guest.c...
tests/gdb/gdb_test_guest.c:85: if (strcmp(argv[1], "call") == 0) {
I left function call to prevent gcc to optimize it away.

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/nacl.scons
File tests/gdb/nacl.scons (right):

http://codereview.chromium.org/10896004/diff/15001/tests/gdb/nacl.scons#newco...
tests/gdb/nacl.scons:73: command=['${PYTHON}', env.File('call.py')],
On 2012/08/30 16:55:57, Mark Seaborn wrote:
> Nit: "call" is a bit generic.  Can you call this something like
"call_from_gdb"
> or "manufactured_call"?  Something to indicate that GDB is doing something a
> little scary when it hijacks a thread and modifies its registers to run a
> function of its choosing. :-)

Done.

[Mark Seaborn, 2012-08-31 16:40:13]

LGTM

http://codereview.chromium.org/10896004/diff/26005/src/trusted/debug_stub/pos...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/26005/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:90: // code via the dynamic code
area the same way nacl_test.c does.
It should be "nacl_text.c"

http://codereview.chromium.org/10896004/diff/26005/tests/gdb/gdb_test_guest.c
File tests/gdb/gdb_test_guest.c (right):

http://codereview.chromium.org/10896004/diff/26005/tests/gdb/gdb_test_guest.c...
tests/gdb/gdb_test_guest.c:86: /* Call function so that it doesn't optimized
away. */
"doesn't get optimized".  However, this shouldn't be necessary:  it shouldn't
get optimised away because:
 1) we don't use link-time optimisation with gcc, and the object file for this
file must include test_call_from_gdb();
 2) we build with -O0.

But also this wouldn't be sufficient in the case of LTO (e.g. with PNaCl),
because the function could get inlined away.

I'm just saying.  You don't necessarily need to change this.

[halyavin, 2012-09-03 10:33:50]

http://codereview.chromium.org/10896004/diff/26005/src/trusted/debug_stub/pos...
File src/trusted/debug_stub/posix/platform_impl.cc (right):

http://codereview.chromium.org/10896004/diff/26005/src/trusted/debug_stub/pos...
src/trusted/debug_stub/posix/platform_impl.cc:90: // code via the dynamic code
area the same way nacl_test.c does.
On 2012/08/31 16:40:13, Mark Seaborn wrote:
> It should be "nacl_text.c"

Done.

http://codereview.chromium.org/10896004/diff/26005/tests/gdb/gdb_test_guest.c
File tests/gdb/gdb_test_guest.c (right):

http://codereview.chromium.org/10896004/diff/26005/tests/gdb/gdb_test_guest.c...
tests/gdb/gdb_test_guest.c:86: /* Call function so that it doesn't optimized
away. */
On 2012/08/31 16:40:13, Mark Seaborn wrote:
> "doesn't get optimized".  However, this shouldn't be necessary:  it shouldn't
> get optimised away because:
>  1) we don't use link-time optimisation with gcc, and the object file for this
> file must include test_call_from_gdb();
>  2) we build with -O0.
> 
> But also this wouldn't be sufficient in the case of LTO (e.g. with PNaCl),
> because the function could get inlined away.
> 
> I'm just saying.  You don't necessarily need to change this.

Done.