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

src/trusted/debug_stub/posix/platform_impl.cc

Index: src/trusted/debug_stub/posix/platform_impl.cc
===================================================================
--- src/trusted/debug_stub/posix/platform_impl.cc	(revision 9590)
+++ src/trusted/debug_stub/posix/platform_impl.cc	(working copy)
@@ -67,25 +67,22 @@
   return SafeMemoryCopy(dst, reinterpret_cast<void*>(virt), len);
 }
 
-bool IPlatform::SetMemory(uint64_t virt, uint32_t len, void *src) {
+bool IPlatform::SetMemory(struct NaClApp* nap, uint64_t virt, uint32_t len,

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

Use the " *" spacing style

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

Done.

+                          void *src) {
   uintptr_t page_mask = NACL_PAGESIZE - 1;
   uintptr_t page = virt & ~page_mask;
   uintptr_t mapping_size = ((virt + len + page_mask) & ~page_mask) - page;
-  if (mprotect(reinterpret_cast<void*>(page), mapping_size,
-               PROT_READ | PROT_WRITE) != 0) {
+  bool is_code = virt + len <= nap->mem_start + nap->dynamic_text_end;
+  if (is_code && mprotect(reinterpret_cast<void*>(page), mapping_size,

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

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.

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

Done.

+                          PROT_READ | PROT_WRITE) != 0) {

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

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.)

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

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.

     return false;
   }
   bool succeeded = SafeMemoryCopy(reinterpret_cast<void*>(virt), src, len);
-  // TODO(mseaborn): We assume here that SetMemory() is being used to
-  // set or remove a breakpoint in the code area, so that PROT_READ |
-  // PROT_EXEC are the correct flags to restore the mapping to.
-  // The earlier mprotect() does not tell us what the original flags
-  // were.  To find this out we could either:
-  //  * read /proc/self/maps (not available inside outer sandbox); or
-  //  * use service_runtime's own mapping tables.
+  // TODO(mseaborn): We use mprotect only to modify code area, so

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

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]

Changed wording a bit to clarify why zero page is different.

+  // PROT_READ | PROT_EXEC are the correct flags to restore the mapping to.
   // Alternatively, we could modify code the same way nacl_text.c does.
-  if (mprotect(reinterpret_cast<void*>(page), mapping_size,
-               PROT_READ | PROT_EXEC) != 0) {
+  if (is_code && mprotect(reinterpret_cast<void*>(page), mapping_size,
+                          PROT_READ | PROT_EXEC) != 0) {
     return false;
   }
   // Flush the instruction cache in case we just modified code to add