If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add AddressSanitizer (ASan) memory check annotations to PLArena

RESOLVED FIXED in 4.10

Status

NSPR
NSPR
P2
enhancement
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: mats, Assigned: Wan-Teh Chang)

Tracking

({sec-other, sec-want})

other
4.10
sec-other, sec-want

Firefox Tracking Flags

(firefox-esr17 wontfix, b2g18 wontfix)

Details

Attachments

(5 attachments, 7 obsolete attachments)

9.15 KB, patch
mats
: review+
decoder
: superreview+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
3.05 KB, patch
Details | Diff | Splinter Review
4.69 KB, patch
mats
: review-
Details | Diff | Splinter Review
1.45 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.97 KB, patch
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Add memory check annotations to PLArena so that Firefox ASan builds
can detect illegal use of the part of the arena that hasn't been
PL_ARENA_ALLOCATE'ed yet.
(Reporter)

Comment 1

5 years ago
Created attachment 717550 [details] [diff] [review]
fix

This change is intended to only affect MOZILLA_CLIENT code.
The idea is to initially mark the malloc'ed memory as NOACCESS and then mark
each chunk that is allocated from the arena as UNDEFINED.
This enables ASan to detect any access to the "avail .. limit" area.
Attachment #717550 - Flags: review?(wtc)
(Assignee)

Comment 2

5 years ago
Comment on attachment 717550 [details] [diff] [review]
fix

Review of attachment 717550 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.

::: nsprpub/lib/ds/plarena.h
@@ +16,5 @@
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#include "../mozilla/MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */
> +#endif

We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
macros in NSPR.
Attachment #717550 - Flags: review?(wtc) → review-
Keywords: sec-other
(Reporter)

Comment 3

5 years ago
(In reply to Wan-Teh Chang from comment #2)
> We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
> macros in NSPR.

It's not clear to me what you're suggesting.  I guess I should copy
mfbt/MemoryChecking.h to nsprpub/lib/ds/MemoryChecking.h and then use:

> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif
> +#include "MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */

If not, how should I get MOZ_ASAN and MOZ_VALGRIND that comes from
--enable-valgrind / --enable-address-sanitizer configure arguments?
Flags: needinfo?(wtc)
(Reporter)

Comment 4

5 years ago
Created attachment 730749 [details] [diff] [review]
fix, v2

No response so I'm assuming that's OK ;-)

https://tbpl.mozilla.org/?tree=Try&rev=479a10ce10c3
Attachment #717550 - Attachment is obsolete: true
Attachment #730749 - Flags: review?(wtc)
Flags: needinfo?(wtc)
(Assignee)

Comment 5

5 years ago
Comment on attachment 730749 [details] [diff] [review]
fix, v2

Review of attachment 730749 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch and sorry for the late review.  I reviewed the patch
carefully today.  I suggest some changes.

Why is this bug marked as security-sensitive and labeled with the sec-other
keyword?

::: mfbt/MemoryChecking.h
@@ +1,1 @@
> +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

When this file is copied to NSPR, it should be renamed.
I think we can rename it plmemcheck.h or plmarkmem.h.
Update the header include guard macro mozilla_MemoryChecking_h_
to match the new file name.

The macros defined in NSPR's copy of this header should
use the PL_ prefix instead of MOZ_.  So they should be
named PL_MAKE_MEM_NOACCESS, PL_MAKE_MEM_UNDEFINED, etc.

@@ +28,4 @@
>  
>  #if defined(MOZ_ASAN) || defined(MOZ_VALGRIND)
>  #define MOZ_HAVE_MEM_CHECKS 1
>  #endif

Delete these three lines from NSPR's copy of this header.

We can add PL_HAVE_MEM_CHECKS when we need it.

::: nsprpub/lib/ds/plarena.h
@@ +15,5 @@
>  #include "prtypes.h"
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif

We need a better way to do this.  We should add --enable-address-sanitizer
and --enable-valgrind to nsprpub/configure.in, similar to how they were
added to js/src/configure.in:
http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in

http://hg.mozilla.org/mozilla-central/rev/ad79ffdf94a3

I also wonder if NSPR should test its own versions of the MOZ_ASAN
and MOZ_VALGRIND macros: PL_ASAN and PL_VALGRIND.  It seems convenient
to just test MOZ_ASAN and MOZ_VALGRIND.  Otherwise any Mozilla code that
uses PLArena will need to be compiled with -DPL_ASAN -DPL_VALGRIND in
addition to -DMOZ_ASAN -DMOZ_VALGRIND.

::: security/coreconf/config.mk
@@ +176,5 @@
>  USE_UTIL_DIRECTLY = 1
>  
> +ifdef MOZILLA_CLIENT
> +DEFINES += -DMOZILLA_CLIENT=1
> +endif

We are trying to use "feature" make variables/C-preprocessor
macros.  So this should be:

# Define MOZ_ASAN or MOZ_VALGRIND for PLArena.
ifdef MOZ_ASAN
DEFINES += -DMOZ_ASAN
endif
ifdef MOZ_VALGRIND
DEFINES += -DMOZ_VALGRIND
endif

There is a way to pass MOZ_ASAN=1 or MOZ_VALGRIND=1 to the
NSS makefiles.  It used to be done in security/manager/Makefile.in.
But that makefile is no longer there. I don't know where it resides
now.
Attachment #730749 - Flags: review?(wtc) → review-
(Reporter)

Comment 6

5 years ago
> Why is this bug marked as security-sensitive and labeled with the sec-other
> keyword?

It's security-sensitive as a precaution because the patch may reveal exploitable
bugs in shipped products.  We shouldn't make it public until we let our fuzz
team run with the patch for a while to confirm that's not the case.

It's sec-other just to give it a security rating - the bug in itself isn't
a security problem, since it's a tool.
(Reporter)

Comment 7

5 years ago
Sorry, but I've lost interest in pursuing this.
I'm working on improving the pres shell arena and I think it's better
use of my time to finish that work (a side-effect is that it will
stop using plarena).
Feel free to use the patch as you see fit.
Assignee: matspal → wtc
(Assignee)

Comment 8

5 years ago
Mats: no problem.

I researched AddressSanitizer and valgrind last night and have a better
understanding of this enhancement request.

If you are mainly interested in AddressSanitizer, the required changes
are small because AddressSanitizer builds can be detected using compiler
pre-defined macros (__has_feature(address_sanitizer) or __SANITIZE_ADDRESS__).
I can adapt your patch easily.  I also know how to add support for valgrind
and I can take care of that.
(In reply to Wan-Teh Chang from comment #8)

> If you are mainly interested in AddressSanitizer, the required changes
> are small because AddressSanitizer builds can be detected using compiler
> pre-defined macros (__has_feature(address_sanitizer) or
> __SANITIZE_ADDRESS__).

Yep, that's right. I think we didn't use this because when I implemented MOZ_ASAN, this way of checking for ASan didn't exist yet, or it wasn't documented/well-known yet. Now all the sanitizers have such a way of detecting at compile-time which could make the --enable-address-sanitizer flag we got, obsolete.
(Assignee)

Comment 10

5 years ago
Created attachment 733658 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, by Mats Palmgren

This adds ASan memory check annotations.  This does not require any
build system changes, so I suggest we do this first.

This patch is based on Mats's patch.
Attachment #730749 - Attachment is obsolete: true
Attachment #733658 - Flags: review?(matspal)
(Assignee)

Updated

5 years ago
Attachment #733658 - Flags: review?(choller)
(Reporter)

Comment 11

5 years ago
Comment on attachment 733658 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, by Mats Palmgren

In the overview comment in plarena.h:
"... the following three macros are provided:
MOZ_MAKE_MEM_NOACCESS ..."

You probably want to change those three to use PL_

Other than that, I'll leave the actual code review for choller@
Attachment #733658 - Flags: review?(matspal)
(Reporter)

Comment 12

5 years ago
And thanks for picking this up Wan-Teh!
ASan support is indeed what I was looking for.
(Assignee)

Comment 13

5 years ago
Created attachment 733664 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v2, by Mats Palmgren

Mats: you're right. The comment should say PL_MAKE_MEM_. Thanks.
Attachment #733658 - Attachment is obsolete: true
Attachment #733658 - Flags: review?(choller)
Attachment #733664 - Flags: review?(choller)

Updated

5 years ago
Keywords: sec-want
Comment on attachment 733664 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v2, by Mats Palmgren

Review of attachment 733664 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, r+ for the ASan stuff. Note that I'm not a peer for NSPR :)

::: lib/ds/plarena.h
@@ +180,5 @@
> +#define PL_CLEAR_ARENA(a) \
> +    PR_BEGIN_MACRO \
> +        PL_MAKE_MEM_UNDEFINED((void*)(a), (a)->limit - (PRUword)(a)); \
> +        memset((void*)(a), PL_FREE_PATTERN, (a)->limit - (PRUword)(a)); \
> +    PR_END_MACRO

Is the MAKE_MEM_UNDEFINED call here because the memory could have been marked NOACCESS before?
Attachment #733664 - Flags: review?(choller) → review+
(Reporter)

Comment 15

5 years ago
> Is the MAKE_MEM_UNDEFINED call here because the memory could have been
> marked NOACCESS before?

Correct.  See FreeArenaList and PL_ClearArenaPool for example.
The whole arena is marked NOACCESS when in arena_freelist, other in-use
arenas are partly NOACCESS, so when freeing/clearing those things, which
a client can do at will afaict, we need to temporarily mark the arena
UNDEFINED to allow for the memset.  In the case of PL_ClearArenaPool,
it is then promptly marked NOACCESS again, and in the case of FreeArenaList
the memory is free'd which also marks it NOACCESS of course.
(Reporter)

Comment 16

5 years ago
Gary, Jesse, can you take this patch for a spin in the fuzzers before it lands?
To find any mistakes in the code, and to avoid zero-daying ourselves.  Thanks.
(Assignee)

Comment 17

5 years ago
decoder,mats: thank you for the review.

I will wait for the fuzzing report from Gary or Jesse before checking
this in.
I haven't been working much on Firefox Asan builds recently, so I'll have to differ here.

Comment 19

5 years ago
Comments in the patch mention Valgrind, but the code only seems to support ASan?
(Assignee)

Comment 20

5 years ago
Jesse: yes, I will add Valgrind support later.
For Valgrind support, just extend the macros as in mfbt/MemoryChecking.h.

Comment 22

5 years ago
I'll give this patch a spin and report back on Monday.
Group: crypto-core-security

Comment 23

5 years ago
ASan DOM fuzzing with the patch didn't uncover any problems over the weekend.
(Assignee)

Comment 24

5 years ago
Created attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

(Please consider the "superreview" request as a second review request.)

I found a bug in the change to PL_ARENA_GROW. You can see my fix by
diff'ing this patch against the v2 patch.

This MXR query shows PL_ARENA_GROW is only used by NSS:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ARENA_GROW

So this bug did not invalidate the ASan DOM fuzzing that Jesse did
last weekend.
Attachment #733664 - Attachment is obsolete: true
Attachment #741561 - Flags: superreview?(choller)
Attachment #741561 - Flags: review?(matspal)
(Assignee)

Comment 25

5 years ago
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

I describe the bug in PL_ARENA_GROW below.

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

In the v2 patch, this line is
    PL_MAKE_MEM_UNDEFINED((void *)_p, _incr); \

_p is _a->avail, which is the next aligned address after the last allocation
from the arena.  There may be a hole between the last allocated block and _p.
Therefore, PL_MAKE_MEM_UNDEFINED((void *)_p, _incr) will leave that hole with
the NOACCESS.
(Assignee)

Comment 26

5 years ago
Created attachment 741571 [details] [diff] [review]
Just FYI: NSS patch

With this NSS patch, I can run the NSS test suite with no ASan errors.

The sqlite change is described in this Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=178677

The NSS secport.c change is necessary because that NSS function is
essentially part of PLArena.

Instructions for running NSS test suite under AddressSanitizer:

1. Install clang 3.3 or later on Linux.

2. Build NSS with this make command line:

make CC="clang -g -fsanitize=address" OPTIMIZER="-O1 -fno-omit-frame-pointer" DEFAULT_COMPILER=clang ZDEFS_FLAG= nss_build_all

3. Run NSS test suite (all.sh) as usual.

4. If there are any ASan errors, the call stacks in the output log do not
have function names. You need to use the asan_symbolize.py script to symbolize
the output log.
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me :)
Attachment #741561 - Flags: superreview?(choller) → superreview+
(Assignee)

Comment 28

5 years ago
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Pushed to the NSPR hg repository (NSPR 4.10):
https://hg.mozilla.org/projects/nspr/rev/608d1530807d
Attachment #741561 - Flags: checked-in+
(Reporter)

Comment 29

5 years ago
(In reply to Wan-Teh Chang from comment #25)
> There may be a hole between the last allocated block and _p.

Good catch!

I think the fix is wrong though.  It should work in ASan since it
doesn't detect a read of an UNDEFINED location that has never
been written to (it seems they are working on it though[1]), but it
will cause UMRs in Valgrind if/when you add support for it.

For the three areas - to old allocated area (A), the alignment gap (B),
the new area at the end (C).  These needs to be treated as such:

A: do not change!
B: mark as UNDEFINED
C: mark as UNDEFINED


[1] http://clang.llvm.org/docs/MemorySanitizer.html
(Reporter)

Comment 30

5 years ago
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

r+ since it should work for ASan, but please add a XXX comment
in the code and file a bug about it so it's not forgotten!
Attachment #741561 - Flags: review?(matspal) → review+
(Assignee)

Comment 31

5 years ago
(In reply to Mats Palmgren [:mats] from comment #29)
>
> For the three areas - to old allocated area (A), the alignment gap (B),
> the new area at the end (C).  These needs to be treated as such:
> 
> A: do not change!
> B: mark as UNDEFINED
> C: mark as UNDEFINED

Mats: thank you for your comment. I believe my fix does exactly what you
specified.

Here is my fix. I omitted the type casts for brevity:

 #define PL_ARENA_GROW(p, pool, size, incr) \
     PR_BEGIN_MACRO \
         ...
+            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

A (old allocated data) is the byte range [p, p + size), which we do not
change.

B (alignment gap) may or may not exist. But if it exists, it will become
part of C (new area at the end) and marked as UNDEFINED below.

C (new area at the end) is the byte range [p + size, p + size + incr),
which we mark as UNDEFINED.

Is my analysis correct?
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 4.10
(Assignee)

Comment 32

5 years ago
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

|p| is typically a void* pointer, so we can't perform pointer
arithmetic on |p| without typecasting.

Would it be clearer if I cast |p| to an unsigned char* pointer
(or even a char* pointer) instead?

            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \
(Reporter)

Comment 33

5 years ago
(In reply to Wan-Teh Chang from comment #31)
> +            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

Ah, I misread the patch the first time, sorry.
Your analysis is correct and this is indeed the right fix.
(Reporter)

Comment 34

5 years ago
(In reply to Wan-Teh Chang from comment #32)
Personally, I think "unsigned char*" would make the code clearer.
(Assignee)

Comment 35

5 years ago
Created attachment 741968 [details] [diff] [review]
Use an unsigned char* typecast for pointer arithmetic
Attachment #741968 - Flags: review?(matspal)
(Assignee)

Comment 36

5 years ago
Created attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

Looking at PL_ARENA_ALLOCATE and PL_ArenaAllocate closer, I
noticed that the |nb| argument that PL_ARENA_ALLOCATE to
PL_ArenaAllocate is the aligned value |_nb|, which may be
larger than the original |nb|.  So when PL_ArenaAllocate
unpoisons the returned memory, it may unpoison more bytes
(the alignment gap) than it should.

This can be fixed by passing the original |nb| to
PL_ArenaAllocate. But I fix this by always unpoisoning
the returned memory in PL_ARENA_ALLOCATE.
Attachment #741968 - Attachment is obsolete: true
Attachment #741968 - Flags: review?(matspal)
Attachment #742137 - Flags: superreview?
Attachment #742137 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #741968 - Flags: superreview?(choller)
Attachment #741968 - Flags: review?(matspal)
(Assignee)

Updated

5 years ago
Attachment #741968 - Flags: superreview?(choller)
Attachment #741968 - Flags: review?(matspal)
(Assignee)

Updated

5 years ago
Attachment #742137 - Flags: superreview?(choller)
Attachment #742137 - Flags: superreview?
Attachment #742137 - Flags: review?(matspal)
Attachment #742137 - Flags: review?
(Reporter)

Comment 37

5 years ago
Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

Another good catch!

But, I don't like removing the annotations for direct calls to
PL_ArenaAllocate.

Can we mark the gap (if any) as NOACCESS in PL_ARENA_ALLOCATE instead?
(directly after the call to PL_ArenaAllocate)
(Assignee)

Comment 38

5 years ago
Created attachment 742472 [details] [diff] [review]
Alternative fix 1 for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

Mats: these MXR queries show there are no direct calls to PL_ArenaAllocate
and its deprecated name PR_ArenaAllocate:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ArenaAllocate
http://mxr.mozilla.org/mozilla-central/ident?i=PR_ArenaAllocate

I inspected PL_ARENA_ALLOCATE and PL_ArenaAllocate, and convinced myself
that it doesn't make sense to call PL_ArenaAllocate directly. This is why
I think it's fine to always mark the allocated memory as UNDEFINED in
PL_ARENA_ALLOCATE.

This patch implements your proposed approach.  I am testing it with NSS
now.

I will attach another alternative patch later today.
Attachment #742472 - Flags: review?(matspal)
(Assignee)

Comment 39

5 years ago
Created attachment 742647 [details] [diff] [review]
Alternative fix 2 for PL_ARENA_ALLOCATE: pass the original |nb| argument to PL_ArenaAllocate

My "alternative fix 1" patch is broken. I can't spot where the bug is.

This is my alternative fix 2. The NSS test suite passed with this patch.
Attachment #742647 - Flags: review?(matspal)
(Assignee)

Comment 40

5 years ago
Created attachment 742652 [details] [diff] [review]
Alternative fix 1, v2, for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

The bug seems to be that the macro argument |nb| needs to be
parenthesized in the expression |_nb - nb|. I found a
PL_ARENA_ALLOCATE call where |nb| is an expression:

    PL_ARENA_ALLOCATE(newp, pool, size + incr);
Attachment #742472 - Attachment is obsolete: true
Attachment #742472 - Flags: review?(matspal)
Attachment #742652 - Flags: review?(matspal)
(Reporter)

Comment 41

5 years ago
Comment on attachment 742652 [details] [diff] [review]
Alternative fix 1, v2, for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

r=mats with a couple of nits below.
I slightly prefer this fix because it allows direct calls to
PL_ArenaAllocate just in case someone other than Mozilla does that.


>+            PL_MAKE_MEM_NOACCESS((void *)(_p + nb), _nb - (nb)); \

Please wrap the first "nb" in () too.

>+            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \

Wrap "size".
Attachment #742652 - Flags: review?(matspal) → review+
(Reporter)

Comment 42

5 years ago
Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

r=mats, but please wrap "nb" and "size" in ().

This looks fine too.
Attachment #742137 - Flags: review?(matspal) → review+
(Reporter)

Comment 43

5 years ago
Comment on attachment 742647 [details] [diff] [review]
Alternative fix 2 for PL_ARENA_ALLOCATE: pass the original |nb| argument to PL_ArenaAllocate

This affects code that isn't memory annotations so I think it's
too invasive for this bug.  I think you should file a separate bug
if you want to do this cleanup (and ask a NSPR peer for review
which I'm not).
Attachment #742647 - Flags: review?(matspal) → review-
Depends on: 866525
Attachment #742137 - Flags: superreview?(choller) → superreview+
(Assignee)

Comment 44

5 years ago
In the hg commit message for
https://hg.mozilla.org/integration/mozilla-inbound/rev/0314d200873a
I referenced this bug by mistake. I meant to reference the NSS bug 866525.

If that changeset is pushed to mozilla-central, please update bug 866525
instead. Thanks.
(Assignee)

Comment 45

4 years ago
Created attachment 752544 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic. v2

I found that none of the PL_Arena* friend functions should be
used directly, so I generalized the warning comment to apply
to all of them.

https://hg.mozilla.org/projects/nspr/rev/119d4ef3679c
Attachment #742137 - Attachment is obsolete: true
Attachment #752544 - Flags: checked-in+
(Assignee)

Comment 46

4 years ago
Marked the bug fixed.  Changed the summary to reflect what was actually
added.  Valgrind support will be added in a future NSPR release.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Summary: Add memory check annotations to PLArena → Add AddressSanitizer (ASan) memory check annotations to PLArena
Group: crypto-core-security
status-firefox-esr17: --- → wontfix
status-b2g18: --- → wontfix

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.