Closed Bug 1001569 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- unaffected
firefox30 + fixed
firefox31 + fixed
firefox32 + fixed
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: gkw, Assigned: dougc)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(3 files)

Attached file 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?
Flags: needinfo?(mrosenberg)
not at all.  Is there a way to get the allocation location for the mismatched free?
Flags: needinfo?(mrosenberg)
(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..
Flags: needinfo?(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.
Flags: needinfo?(jseward)
I ran with --keep-stacktraces=alloc-and-free, is it sufficient?

(The stacks look similar to me...)
Flags: needinfo?(mrosenberg)
Flags: needinfo?(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.
Flags: needinfo?(jseward)
Group: javascript-core-security
Assignee: nobody → nihsanullah
Marty: does this Valgrind warning make more sense with Julian's (incomplete) stack traces in comment 5?
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?
Attachment #8420915 - Flags: feedback?(gary)
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)
Attachment #8420915 - Flags: feedback?(gary) → feedback+
Attachment #8420915 - Flags: review?(jorendorff)
This bug is a regression in Firefox 30 from bug 971208. We should uplift this fix to Aurora 31 and Beta 30.
Assignee: nihsanullah → dtc-moz
Blocks: 971208
Keywords: regression
Attachment #8420915 - Flags: review?(jorendorff) → review+
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.
Attachment #8420915 - Flags: sec-approval?
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.
Attachment #8420915 - Flags: sec-approval? → sec-approval+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba4254c2c965
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Group: javascript-core-security
Please nominate for uplift to branches so this can get into this week's beta build.
Flags: needinfo?(dtc-moz)
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
Attachment #8420915 - Flags: approval-mozilla-release?
Attachment #8420915 - Flags: approval-mozilla-beta?
Flags: needinfo?(dtc-moz)
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
Attachment #8420915 - Flags: approval-mozilla-release? → approval-mozilla-aurora?
Attachment #8420915 - Flags: approval-mozilla-beta?
Attachment #8420915 - Flags: approval-mozilla-beta+
Attachment #8420915 - Flags: approval-mozilla-aurora?
Attachment #8420915 - Flags: approval-mozilla-aurora+
Another one for you to verify, Gary.
Flags: needinfo?(gary)
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.
Flags: needinfo?(gary)
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
QA Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.