1001569 - Valgrind detects Mismatched free() / delete / delete with testcase involving YARR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

r = /(?!(W+)|[)+?81,}=|3!(uDS{:]+)/ym
print(r.exec())

(tested on m-c rev e97ec17231f9 with the patch in bug 990247 comment 12 on Ubuntu 14.04, this also needs Valgrind compiled from SVN tip)

$ valgrind --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --show-leak-kinds=definite ./js-opt-32-dm-vg-ts-hfp-linux-5ecd532a167e-990247-c12-e97ec17231f9 --no-asmjs --baseline-eager --no-ion testcase.js

==14003== Mismatched free() / delete / delete []
==14003==    at 0x482FF74: operator delete(void*) (vg_replace_malloc.c:503)
==14003==    by 0x2DDD6D: JSC::Yarr::jitCompile(JSC::Yarr::YarrPattern&, JSC::Yarr::YarrCharSize, JSC::Yarr::JSGlobalData*, JSC::Yarr::YarrCodeBlock&, JSC::Yarr::YarrJITCompileMode) (SegmentedVector.h:207)
==14003==    by 0x2495D9: js::RegExpShared::compile(JSContext*, JSLinearString&, bool) (RegExpObject.cpp:485)
==14003==    by 0x24A98F: js::RegExpShared::compile(JSContext*, bool) (RegExpObject.cpp:460)
==14003==    by 0x24A9D7: js::RegExpShared::compileIfNecessary(JSContext*) (RegExpObject.cpp:509)
/snip

My configure flags are:

AR=ar sh /home/odroid/trees/mozilla-central/js/src/configure --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR options>

Mismatched frees sound bad - tentatively setting sec-critical pending further analysis.

Marty, any idea what's going on here?

Comment 1

[Marty Rosenberg [:mjrosenb]]

not at all.  Is there a way to get the allocation location for the mismatched free?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> not at all.  Is there a way to get the allocation location for the
> mismatched free?

Setting needinfo? from Julian..

Comment 3

[Julian Seward [:jseward]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> Is there a way to get the allocation location for the mismatched free?

Possibly --keep-stacktraces=alloc-and-free

    --keep-stacktraces=alloc|free|alloc-and-free|alloc-then-free|none
        stack trace(s) to keep for malloc'd/free'd areas       [alloc-then-free]

Am not 100% sure this will do what you want, but is worth a try.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Stack with --keep-stacktraces=alloc-and-free

I ran with --keep-stacktraces=alloc-and-free, is it sufficient?

(The stacks look similar to me...)

Comment 5

[Julian Seward [:jseward]]

Looks like this is a plausible complaint.  Allocation stack is confusing
because of so much inlining, but comes to

  ARMAssembler.h:955 loadBranchTarget()
  ARMAssembler.h:916 ensureSpace()
  AssemblerBufferWithConstantPool.h:122 ensureSpace()
  AssemblerBuffer.h:78 ensureSpace()
  AssemblerBuffer.h:243 grow()
  Utility.h:105 js_malloc()
    "return malloc(bytes)"

Freeing point is

  SegmentedVector.h:207
    "delete m_segments[i]"

At least, if my quick look around was correct.  With the malloc/free arrangements
we have, I don't think this is dangerous.  But it does make the code perhaps a
little less clear -- using delete gives the impression there is a destructor to
run, when afaics, there isn't -- so this should be fixed.

Comment 6

[Chris Peterson [:cpeterson]]

Marty: does this Valgrind warning make more sense with Julian's (incomplete) stack traces in comment 5?

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Free Segment's using js_delete to match their allocation with js_new.

Might have bump into this too, in a different test. This might help:

==11299== Mismatched free() / delete / delete []
==11299==    at 0x4028F11: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11299==    by 0x891CFA3: WTF::SegmentedVector<int, 64u>::deleteAllSegments() (SegmentedVector.h:207)
==11299==    by 0x891AB82: WTF::SegmentedVector<int, 64u>::~SegmentedVector() (SegmentedVector.h:119)
==11299==    by 0x891A32F: JSC::ARMAssembler::~ARMAssembler() (ARMAssembler.h:129)
...
==11299==  Address 0xb03d520 is 0 bytes inside a block of size 276 alloc'd
==11299==    at 0x4027AB1: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11299==    by 0x8916E0D: js_malloc(unsigned int) (Utility.h:105)
==11299==    by 0x891705D: js::Vector<int, 64u, js::SystemAllocPolicy>* js_new<js::Vector<int, 64u, js::SystemAllocPolicy> >() (Utility.h:333)
==11299==    by 0x891A7BA: void WTF::SegmentedVector<int, 64u>::append<int>(int const&) (SegmentedVector.h:153)
==11299==    by 0x8918A7C: JSC::ARMAssembler::loadBranchTarget(int, JSC::ARMAssembler::Condition, int) (ARMAssembler.h:958)
...

The problem appears to be that the Segments are allocated using js_new() but are being freed using 'delete' in deleteAllSegments().  Does this patch avoid the valgrind error message on the reported test?

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8420915 [details] [diff] [review]
Free Segment's using js_delete to match their allocation with js_new.

Yes, this patch does seem to make the issue go away! :)

(note that I tested on debug shells rather than opt shells, but I could reproduce the issue without the patch, vs not reproduce issue after the patch)

Comment 9

[Chris Peterson [:cpeterson]]

This bug is a regression in Firefox 30 from bug 971208. We should uplift this fix to Aurora 31 and Beta 30.

Comment 10

[Douglas Crosher [:dougc]]

Comment on attachment 8420915 [details] [diff] [review]
Free Segment's using js_delete to match their allocation with js_new.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It has only been report as an error by Valgrind.  Not sure if it could even be exploited.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, the security issue is not clear to me, although the bug is.


Which older supported branches are affected by this flaw?

Aurora, Beta.


If not all supported branches, which bug introduced the flaw?

Bug 971208.


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to backport.


How likely is this patch to cause regressions; how much testing does it need?

It appears to be a low risk.  Tested locally.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8420915 [details] [diff] [review]
Free Segment's using js_delete to match their allocation with js_new.

sec-approval+ for trunk.

Please prepare and nominate Aurora and Beta patches. Once trunk is green, we can get this in on branches.

Comment 12

[Julian Seward [:jseward]]

The central question is: do we have an allocator setup in which memory
acquired from malloc can cause a crash when freed with delete?  I
don't think we do, but I don't know for sure.  My impression is that
our calls to delete just hand the pointer onwards to free() anyway, so
nothing bad happens.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4254c2c965

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba4254c2c965

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift to branches so this can get into this week's beta build.

Comment 16

[Douglas Crosher [:dougc]]

Comment on attachment 8420915 [details] [diff] [review]
Free Segment's using js_delete to match their allocation with js_new.

[Approval Request Comment]

Regression caused by (bug #): Bug 971208.

User impact if declined: Low, and there might be no impact, it could be harmless.

Testing completed (on m-c, etc.): Tested on m-c.

Risk to taking this patch (and alternatives if risky): Low.

String or IDL/UUID changes made by this patch: n/a

Comment 17

[Douglas Crosher [:dougc]]

Comment on attachment 8420915 [details] [diff] [review]
Free Segment's using js_delete to match their allocation with js_new.

Sorry, did not intend to request approval for 'release' just beta and aurora.

[Approval Request Comment]

Regression caused by (bug #): Bug 971208.

User impact if declined: Low, and there might be no impact, it could be harmless.

Testing completed (on m-c, etc.): Tested on m-c.

Risk to taking this patch (and alternatives if risky): Low.

String or IDL/UUID changes made by this patch: n/a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf3565e8a801
https://hg.mozilla.org/releases/mozilla-beta/rev/18d40554e01f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/18d40554e01f

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Another one for you to verify, Gary.

Comment 21

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Unable to verify this with much certainty - strangely I was unable to reproduce the original issue, it may have been hard to reproduce. Nevertheless I've checked it does not reproduce on m-c now.

Comment 22

[Kamil Jozwiak [:kjozwiak]]

I couldn't reproduce the original issue either. Talked to Gary on IRC and used changeset 5ecd532a167e (the one in comment #0 is incorrect) + the patch from bug 990247 comment 12. Using all the flags mentioned in comment #0, I couldn't reproduce. However, I'm going to mark this as [qa-] do to not being able to verify the issue with certainty.

m-c fx33 [75db55f6fd2c] - couldn't reproduce the issue
aurora fx32 [2a833eea4a52] - couldn't reproduce the issue
beta fx31 [6befadcaa685] - couldn't reproduce the issue
release fx30 [650879b8b29d] - couldn't reproduce the issue