348798 - valgrind arena annotations

Status: RESOLVED DUPLICATE of bug 1061812

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

This patch provides preliminary (and possibly wrong) valgrind annotations for mozilla. The annotations teach valgrind about the fine structure of the memory pools in the nspr plarena code, and more importantly in the js arenas (both normal and GC). 

The annotations are harmless and can be left on in a production build; they cause a few NOPs to be inserted in the instruction stream, but they inform valgrind of memory state transitions if the program is running under valgrind's translation.

I've marked this security-sensitive because it might reveal quite a number of UMRs. Currently I think most of the UMRs it spews at me are my own fault -- errors in the patch -- but you never know. I found it quite a challenge to write the patch in the first place, so review in excruciating detail would be much appreciated.

You will need to have a very recent valgrind to use this patch in production (SVN trunk plus a patch I posted today to the valgrind newsgroup). Hopefully the next point release of valgrind will incorporate these changes.

Comment 1

[Graydon Hoare :graydon]

Attachment: annotation patch

Comment 2

[Graydon Hoare :graydon]

Attachment: corrected patch; previous patch was an interdiff

Comment 3

[Graydon Hoare :graydon]

Attachment: incorporate initial corrections from brendan

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 233911 [details] [diff] [review]
incorporate initial corrections from brendan

>@@ -182,12 +189,20 @@
>                 SET_HEADER(pool, a, ap);
>             } else {
>                 a->base = a->avail = JS_ARENA_ALIGN(pool, a + 1);
>+            } 
>+#ifdef VALGRIND_HINTS
>+            if (malloced) {
>+                VALGRIND_MAKE_MEM_NOACCESS(a->base, 
>+                                           JS_UPTRDIFF(a->limit, a->base));

Nit: JS_UPTRDIFF is more for pointer differencing to compute byte distance, but with jsuword a->limit and a->base, you could just subtract.

>@@ -264,8 +284,10 @@
>      * If p points to an oversized allocation, it owns an entire arena, so we
>      * can simply realloc the arena.
>      */
>-    if (size > pool->arenasize)
>-        return JS_ArenaRealloc(pool, p, size, incr);
>+    if (size > pool->arenasize) {
>+        newp = JS_ArenaRealloc(pool, p, size, incr);
>+        return newp;
>+    }

Revert gratuitous change?

>@@ -444,6 +465,9 @@
>         arenaList->lastLimit = (uint16)(a->prev ? GC_THINGS_SIZE : 0);
>     *ap = a->prev;
> 
>+    /* REVIEW ME: I believe this sets the access bits back to true,
>+       but maybe not? */
>+    VALGRIND_DESTROY_MEMPOOL(a);    

Nit: trailing whitespace detected, purple alert! :-P

I don't understand the REVIEW ME question -- what access bits?

>@@ -2812,6 +2842,12 @@
>                     METER(++arenaList->stats.nthings);
>                 } else {
>                     thing = (JSGCThing *)(firstPage + offset);
>+                    VALGRIND_MEMPOOL_FREE(a, thing);
>+                    /* We've disassociated the chunk from the arena,
>+                     but we will need to make the first two words of
>+                     it as addressable (but undefined), so that it can
>+                     be inserted into a freelist. */
>+                    VALGRIND_MAKE_MEM_UNDEFINED(thing, sizeof(JSGCThing));

Comment style deviation nit (blank line before major comment with stacked stars would match prevailing style).

>--- js/src/jstypes.h	e078a535274aa446d70499c4a980b0364b333e54
>+++ js/src/jstypes.h	f6eec4e2792c6e97274bfc4f53caee7d468b7674
>@@ -439,6 +439,22 @@
> #define JS_UNLIKELY(x)  (x)
> #endif
> 
>+#define VALGRIND_HINTS
>+
>+#ifdef VALGRIND_HINTS
>+#include <valgrind/valgrind.h>
>+#include <valgrind/memcheck.h>
>+#else
>+#define VALGRIND_DESTROY_MEMPOOL(pool)
>+#define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)
>+#define VALGRIND_MEMPOOL_ALLOC(pool, addr, size)
>+#define VALGRIND_MEMPOOL_FREE(pool, addr)
>+#define VALGRIND_MEMPOOL_TRIM(pool, addr, size)
>+
>+#define VALGRIND_MAKE_MEM_NOACCESS(addr, size)
>+#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size)
>+#endif
>+
> /***********************************************************************
> ** MACROS:      JS_ARRAY_LENGTH
> **              JS_ARRAY_END

Nit: put the VALGRIND stuff at the very bottom?

Looks good to me, wtc should look at the plarena changes and igor should check the jsgc ones.

/be

Comment 5

[Brendan Eich [:brendan]]

How're the results with the latest patch?

/be

Comment 6

[Graydon Hoare :graydon]

Attachment: update valgrind hint patch

This covers a dozen or more fiddly little errors in the previous patch (alignment, misinterpreted conditions, failure to preserve V/A bits, etc.)

Firefox seems to be able to browse websites now without spewing an infinite supply of UMRs related to js/pl/gc arenas. There are, alas, a fair number elsewhere (nss, cairo, networking) but I don't think most of them are mine. Yet. Still some work to do here, but it's getting better.

It requires Yet Another Patch To Valgrind Head, posted to the valgrind mailing list tonight.

Comment 7

[Brendan Eich [:brendan]]

Do you want to start posting results, so others can look for bugs in parallel?

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 235524 [details] [diff] [review]
update valgrind hint patch


>+#ifdef VALGRIND_HINTS
>+
>+    /* If we have an "oversized arena", it looks roughly like this:

Nit: start the major comment as usual, with "/*" on its own line and the * stacked over the rest.  No need for blank line between #ifdef and /* but one before the #ifdef might be nice.


>+    if (size > pool->arenasize) {
>+        VALGRIND_MAKE_MEM_UNDEFINED((jsuword)a + sizeof(*a), 
>+                                    JS_UPTRDIFF(PTR_TO_HEADER(pool, a->base),
>+                                                ((jsuword)a + sizeof(*a))));
>+    }
>+    if (a->limit > a->avail) {
>+        VALGRIND_MAKE_MEM_UNDEFINED(a->avail, a->limit - a->avail);
>+    }

Uber-nit: no need for braces around one-line then in prevailing style.

>+#ifdef VALGRIND_HINTS
>+    /* Mark the unallocated and "alignment slop" regions as
>+       unaddressable, again. */

Nit: use major comment style here.

>+    if (size > pool->arenasize) {
>+        VALGRIND_MAKE_MEM_NOACCESS((jsuword)a + sizeof(*a), 
>+                                   JS_UPTRDIFF(PTR_TO_HEADER(pool, a->base),
>+                                               ((jsuword)a + sizeof(*a))));
>+    }
>+    if (a->limit > a->avail) {
>+        VALGRIND_MAKE_MEM_NOACCESS(a->avail, a->limit - a->avail);
>+    }

Repeated uber-nit.

>+#ifdef VALGRIND_HINTS
>+        JSArena *a = &pool->first;
>+        while (! ((a->base <= (jsuword)p) && ((jsuword)p < a->avail)))

Nit: over parenthesized relational expressions, but better way out is to use

          while (JS_UPTRDIFF(p, a->base) >= a->avail - a->base)

instead.

>+            a = a->next;
>+#endif
>         memcpy(newp, p, size);
>+        /* We could make this a VALGRIND_MEMPOOL_FREE, but the other

Nit: blank line and major comment start on its own line.

>+                VALGRIND_CHECK_MEM_IS_ADDRESSABLE(p, size+incr);              \

Ultra-nit: size + incr would breathe a bit more consonantly with prevailing style ;-).


>+#ifdef VALGRIND_HINTS
>+    jsuword loFlagSz, hiFlagSz;
>+#endif

Here's where I remember to advocate lowFlagSize and highFlagSize, contrary to my Unix hax0r past.


>@@ -2750,11 +2778,13 @@ restart:
>                 if (flags & GCF_MARK) {
>                     *flagp &= ~GCF_MARK;
>                 } else if (!(flags & (GCF_LOCK | GCF_FINAL))) {
>+
>+                    thing = (JSGCThing *)(firstPage + offset);
>+

Nit: no need for blank line before the non-blank line here.

Pick these nits and request Igor's review on the jsgc changes, and you can check in the js stuff any time.  Thanks,

/be

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

+#include <valgrind/valgrind.h>
+#include <valgrind/memcheck.h>

shouldn't there be configure checks for those headers?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 235524 [details] [diff] [review]
update valgrind hint patch

May I request that these patches be produced in such a way that
bugzilla's patch diff tool can make sense of them?  e.g.
https://bugzilla.mozilla.org/attachment.cgi?id=235524&action=diff

That tool is unable to determine that this patch patches more than 
one source file, nor which versions of the files are being patched.
I think it wants lines that begin with "Index: <filename>" and 
also CVS version numbers in the date/timestamp lines.

Thanks.

Comment 11

[Graydon Hoare :graydon]

(In reply to comment #9)
> +#include <valgrind/valgrind.h>
> +#include <valgrind/memcheck.h>
> 
> shouldn't there be configure checks for those headers?

They are redistributable headers, containing standalone definitions of instruction sequences that interface with valgrind (when running under supervision). They do not represent a build-time dependency on external libraries, so are meant to be incorporated into the source distribution of any product that uses them; i.e. added to mozilla CVS. 

I have not included them in the patch yet, but my intention is to add them to the final form of the patch.

If reviewers are uncomfortable with this strategy, of course, we can also make them build-time external dependencies with configury checks. I was just assuming we would integrate.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

oh ok. I didn't realize that you wanted to check them into mozilla CVS. that's fine by me.

Comment 13

[timeless]

could you also annotate nsFixedSizeAllocator?

Comment 14

[timeless]

Comment on attachment 235524 [details] [diff] [review]
update valgrind hint patch

please consider using (JS|PR)_(BEGIN+END)_MACRO for your various empty statements.

>+        VALGRIND_CHECK_MEM_IS_ADDRESSABLE(p, _nb);                            \
>     JS_END_MACRO

>+++ js/src/jstypes.h	712cf84d8bc5b5c4cd78de52e713ef5429cc6832
>+#define VALGRIND_DESTROY_MEMPOOL(pool)
>+#define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)
>+#define VALGRIND_MEMPOOL_ALLOC(pool, addr, size)
>+#define VALGRIND_MEMPOOL_FREE(pool, addr)
>+#define VALGRIND_MEMPOOL_TRIM(pool, addr, size)
>+#define VALGRIND_MOVE_MEMPOOL(poolA, poolB)
>+#define VALGRIND_MEMPOOL_CHANGE(pool, addrA, addrB, size)
>+#define VALGRIND_MAKE_MEM_NOACCESS(addr, size)
>+#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size)
>+#define VALGRIND_CHECK_MEM_IS_ADDRESSABLE(addr, size)
>+#define VALGRIND_CHECK_MEM_IS_DEFINED(addr, size)

>+++ nsprpub/lib/ds/plarena.h	a9e37749ec02ad5d42fa854a5c89ccd11ea56e8f
>+#define VALGRIND_DESTROY_MEMPOOL(pool)
>+#define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)
>+#define VALGRIND_MEMPOOL_ALLOC(pool, addr, size)
>+#define VALGRIND_MEMPOOL_FREE(pool, addr)
>+#define VALGRIND_MEMPOOL_TRIM(pool, addr, size)
>+#define VALGRIND_MOVE_MEMPOOL(poolA, poolB)
>+#define VALGRIND_MEMPOOL_CHANGE(pool, addrA, addrB, size)
>+#define VALGRIND_MAKE_MEM_NOACCESS(addr, size)
>+#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size)

...
>     PR_END_MACRO

Comment 15

[Graydon Hoare :graydon]

Attachment: update valgrind hint patch

Incorporate suggested changes, fix several annotation bugs. Requires patch posted to valgrind ML on Sept 8. Seems to be working reasonably well now (jsval-mangled GCTHING pointers report as internal -- "possibly leaked" -- but this is easy to filter out while skimming leak reports).

Comment 16

[Igor Bukanov]

Adding dependency on bug 351602 to remember to update the thread-local freelist annotations when the patch is ready.

Comment 17

[Brendan Eich [:brendan]]

Is this all but done, and an updated patch ready to land?

/be

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Do you really propose to unconditionally define VALGRIND_HINTS in 
nsprpub's plarena.h?

Comment 19

[Wan-Teh Chang]

Comment on attachment 237515 [details] [diff] [review]
update valgrind hint patch

Graydon, please remove "#define VALGRIND_HINTS" from jstypes.h
and plarena.h.

Comment 20

[Jesse Ruderman]

Does this patch need to be secret?  I'd rather it be in the tree than bitrot in a security-sensitive bug.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237515 [details] [diff] [review]
update valgrind hint patch

Comments:
1. I see no reason for this bug to be security sensitive.  
The bug and the patch do not, by themselves, reveal any exploitable defect.
People who are capable of runing valgrind are capable of 
finding UMRs without this bug.

2. I'm OK with the patch for the two files in nsprpub,
and would give that part r+ separately from the rest, 
if I could, except for one thing.  It is absolutely 
NOT OK to unconditionally #define VALGRIND_HINTS in 
nsprpub/lib/ds/plarena.h because not all supported 
platforms have valgrind available, or installed.  
Valgrind cannot become a necessary prerequisite to 
building NSPR.  

Also, wouldn't it be better to #include "valgrind.h"
rather than <valgrind/valgrind.h> (which assumes that
valgrind's headers are installed in /usr/include/valgrind)
and let the user specify -I $PATH_TO_VALGRIND_HEADERS ?

Suggestion, submit a separate patch for NSPR that 
patches cleanly against the trunk and doesn't define
VALGRIND_HINTS, and ask for review.