Closed
Bug 348798
Opened 18 years ago
Closed 6 years ago
valgrind arena annotations
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1061812
People
(Reporter: graydon, Assigned: graydon)
References
Details
(Keywords: sec-want, valgrind, Whiteboard: [sg:want P3])
Attachments
(1 file, 4 obsolete files)
28.45 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
Attachment #233907 -
Attachment is obsolete: true
Assignee | ||
Comment 3•18 years ago
|
||
Attachment #233910 -
Attachment is obsolete: true
Comment 4•18 years ago
|
||
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•18 years ago
|
||
How're the results with the latest patch? /be
Assignee | ||
Comment 6•18 years ago
|
||
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.
Attachment #233911 -
Attachment is obsolete: true
Comment 7•18 years ago
|
||
Do you want to start posting results, so others can look for bugs in parallel? /be
Comment 8•18 years ago
|
||
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•18 years ago
|
||
+#include <valgrind/valgrind.h> +#include <valgrind/memcheck.h> shouldn't there be configure checks for those headers?
Comment 10•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
(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•18 years ago
|
||
oh ok. I didn't realize that you wanted to check them into mozilla CVS. that's fine by me.
Comment 13•18 years ago
|
||
could you also annotate nsFixedSizeAllocator?
Comment 14•18 years ago
|
||
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
Assignee | ||
Comment 15•18 years ago
|
||
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).
Attachment #235524 -
Attachment is obsolete: true
Comment 16•18 years ago
|
||
Adding dependency on bug 351602 to remember to update the thread-local freelist annotations when the patch is ready.
Depends on: 351602
Comment 17•17 years ago
|
||
Is this all but done, and an updated patch ready to land? /be
Comment 18•17 years ago
|
||
Do you really propose to unconditionally define VALGRIND_HINTS in nsprpub's plarena.h?
Comment 19•17 years ago
|
||
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•17 years ago
|
||
Does this patch need to be secret? I'd rather it be in the tree than bitrot in a security-sensitive bug.
Comment 21•17 years ago
|
||
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.
Updated•17 years ago
|
Whiteboard: [sg:want P3]
Updated•17 years ago
|
Group: security
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•