Fix handling of mprotect so as to be more consistent with the handling

of mmap.  Fixes #205541 and its dup #210268.  The fix is simple enough
but the analysis is a bit complex, as detailed in comments.

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@11031

diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c
index c262d69d761b419c18e591c639a841897fa3214a..119cf2339f07bef3c7ae2e954b4b795be4e43f27 100644 (file)
--- a/memcheck/mc_main.c
+++ b/memcheck/mc_main.c
@@ -1636,6 +1636,22 @@ static void make_mem_defined_if_addressable ( Addr a, SizeT len )
    }
 }
 
+/* Similarly (needed for mprotect handling ..) */
+static void make_mem_defined_if_noaccess ( Addr a, SizeT len )
+{
+   SizeT i;
+   UChar vabits2;
+   DEBUG("make_mem_defined_if_noaccess(%p, %llu)\n", a, (ULong)len);
+   for (i = 0; i < len; i++) {
+      vabits2 = get_vabits2( a+i );
+      if (LIKELY(VA_BITS2_NOACCESS == vabits2)) {
+         set_vabits2(a+i, VA_BITS2_DEFINED);
+         if (UNLIKELY(MC_(clo_mc_level) >= 3)) {
+            MC_(helperc_b_store1)( a+i, 0 ); /* clear the origin tag */
+         } 
+      }
+   }
+}
 
 /* --- Block-copy permissions (needed for implementing realloc() and
        sys_mremap). --- */
@@ -3701,16 +3717,86 @@ void check_mem_is_defined_asciiz ( CorePart part, ThreadId tid,
    }
 }
 
+/* Handling of mmap and mprotect is not as simple as it seems.
+
+   The underlying semantics are that memory obtained from mmap is
+   always initialised, but may be inaccessible.  And changes to the
+   protection of memory do not change its contents and hence not its
+   definedness state.  Problem is we can't model
+   inaccessible-but-with-some-definedness state; once we mark memory
+   as inaccessible we lose all info about definedness, and so can't
+   restore that if it is later made accessible again.
+
+   One obvious thing to do is this:
+
+      mmap/mprotect NONE  -> noaccess
+      mmap/mprotect other -> defined
+
+   The problem case here is: taking accessible memory, writing
+   uninitialised data to it, mprotecting it NONE and later mprotecting
+   it back to some accessible state causes the undefinedness to be
+   lost.
+
+   A better proposal is:
+
+     (1) mmap NONE       ->  make noaccess
+     (2) mmap other      ->  make defined
+
+     (3) mprotect NONE   ->  # no change
+     (4) mprotect other  ->  change any "noaccess" to "defined"
+
+   (2) is OK because memory newly obtained from mmap really is defined
+       (zeroed out by the kernel -- doing anything else would
+       constitute a massive security hole.)
+
+   (1) is OK because the only way to make the memory usable is via
+       (4), in which case we also wind up correctly marking it all as
+       defined.
+
+   (3) is the weak case.  We choose not to change memory state.
+       (presumably the range is in some mixture of "defined" and
+       "undefined", viz, accessible but with arbitrary V bits).  Doing
+       nothing means we retain the V bits, so that if the memory is
+       later mprotected "other", the V bits remain unchanged, so there
+       can be no false negatives.  The bad effect is that if there's
+       an access in the area, then MC cannot warn; but at least we'll
+       get a SEGV to show, so it's better than nothing.
+
+   Consider the sequence (3) followed by (4).  Any memory that was
+   "defined" or "undefined" previously retains its state (as
+   required).  Any memory that was "noaccess" before can only have
+   been made that way by (1), and so it's OK to change it to
+   "defined".
+
+   See https://bugs.kde.org/show_bug.cgi?id=205541
+   and https://bugs.kde.org/show_bug.cgi?id=210268
+*/
 static
 void mc_new_mem_mmap ( Addr a, SizeT len, Bool rr, Bool ww, Bool xx,
                        ULong di_handle )
 {
-   if (rr || ww || xx)
+   if (rr || ww || xx) {
+      /* (2) mmap/mprotect other -> defined */
       MC_(make_mem_defined)(a, len);
-   else
+   } else {
+      /* (1) mmap/mprotect NONE  -> noaccess */
       MC_(make_mem_noaccess)(a, len);
+   }
 }
 
+static
+void mc_new_mem_mprotect ( Addr a, SizeT len, Bool rr, Bool ww, Bool xx )
+{
+   if (rr || ww || xx) {
+      /* (4) mprotect other  ->  change any "noaccess" to "defined" */
+      make_mem_defined_if_noaccess(a, len);
+   } else {
+      /* (3) mprotect NONE   ->  # no change */
+      /* do nothing */
+   }
+}
+
+
 static
 void mc_new_mem_startup( Addr a, SizeT len,
                          Bool rr, Bool ww, Bool xx, ULong di_handle )
@@ -5780,7 +5866,7 @@ static void mc_pre_clo_init(void)
    //
    // So we should arguably observe all this.  However:
    // - The current inaccuracy has caused maybe one complaint in seven years(?)
-   // - Telying on the zeroed-ness of whole brk'd pages is pretty grotty... I
+   // - Relying on the zeroed-ness of whole brk'd pages is pretty grotty... I
    //   doubt most programmers know the above information.
    // So I'm not terribly unhappy with marking it as undefined. --njn.
    //
@@ -5790,21 +5876,15 @@ static void mc_pre_clo_init(void)
    // just mark all memory it allocates as defined.]
    //
    VG_(track_new_mem_brk)         ( make_mem_undefined_w_tid );
+
+   // Handling of mmap and mprotect isn't simple (well, it is simple,
+   // but the justification isn't.)  See comments above, just prior to
+   // mc_new_mem_mmap.
    VG_(track_new_mem_mmap)        ( mc_new_mem_mmap );
+   VG_(track_change_mem_mprotect) ( mc_new_mem_mprotect );
    
    VG_(track_copy_mem_remap)      ( MC_(copy_address_range_state) );
 
-   // Nb: we don't do anything with mprotect.  This means that V bits are
-   // preserved if a program, for example, marks some memory as inaccessible
-   // and then later marks it as accessible again.
-   // 
-   // If an access violation occurs (eg. writing to read-only memory) we let
-   // it fault and print an informative termination message.  This doesn't
-   // happen if the program catches the signal, though, which is bad.  If we
-   // had two A bits (for readability and writability) that were completely
-   // distinct from V bits, then we could handle all this properly.
-   VG_(track_change_mem_mprotect) ( NULL );
-      
    VG_(track_die_mem_stack_signal)( MC_(make_mem_noaccess) ); 
    VG_(track_die_mem_brk)         ( MC_(make_mem_noaccess) );
    VG_(track_die_mem_munmap)      ( MC_(make_mem_noaccess) );