All users were logged out of Bugzilla on October 13th, 2018

Write after free related to mozilla::TracerRunnable::~TracerRunnable()

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

({sec-moderate, valgrind})

Trunk
mozilla18
ARM
Android
sec-moderate, valgrind
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

(Whiteboard: [adv-main18+])

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Multiple read- and write-after-free and double free errors observed at
shutdown on Fennec.

STR: build with profiler (SPS) support.  Start Fennec.  Start the
profiler using "Toggle Sampling" in the main menu.  Then use main menu
to quit.  There are multiple invalid memory accesses, ending in a
segfault (which may or may not be related).
(Assignee)

Comment 1

6 years ago
Created attachment 665870 [details]
Valgrind trace

Here is the V output.  Although there are a whole bunch of errors
reported, it is clear that they are all accesses to the same 24-byte
block that has already been freed.  So I think these all result from
the same underlying problem.
(Assignee)

Comment 2

6 years ago
Probably caused by incorrect use of nsCOMPtr, in CleanUpWidgetTracing
in widget/android/AndroidBridge.cpp (thanks Ms2get for confirmation):

    nsCOMPtr<TracerRunnable> sTracerRunnable;

    bool InitWidgetTracing() {
        if (!sTracerRunnable)
            sTracerRunnable = new TracerRunnable();
        return true;
    }

    void CleanUpWidgetTracing() {
        if (sTracerRunnable)
            delete sTracerRunnable;
        sTracerRunnable = nullptr;
    }
(Assignee)

Updated

6 years ago
Blocks: 779291
Any reason you haven't flagged review? This patch seems like it fixes a clear problem and it's ready to land.

Updated

6 years ago
Attachment #666943 - Flags: review?(blassey.bugs)
Ok blassey can review this, turns out I needed to find an intersection between the android widget peers and someone who can access this bug.
Attachment #666943 - Flags: review?(blassey.bugs) → review+
Keywords: checkin-needed
Assignee: nobody → jseward
Status: NEW → ASSIGNED
When did this regress? If it regressed sometime ago and affects the branches, we may first have to request sec-approval.
The event tracer is only used by profiling and a few limited internal tools. While this code as shipped I don't think it's worth uplifting.
Setting sec-low based on comment 8. sec-approval does not need to be requested on the patch, so it can land after Try turns green.
Keywords: sec-low
https://hg.mozilla.org/mozilla-central/rev/24b61485c945
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
status-firefox-esr10: --- → unaffected
Keywords: sec-low → sec-moderate
Did this affect Firefox 17 (and, therefore ESR 17)?
Whiteboard: [adv-main18+]
Group: core-security
status-firefox-esr17: --- → unaffected
You need to log in before you can comment on or make changes to this bug.