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

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

({sec-moderate, valgrind})

Trunk
mozilla18
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main18+])

Attachments

(2 attachments)

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).
Posted file 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.
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;
    }
Posted patch fixSplinter Review
Blocks: 779291
Any reason you haven't flagged review? This patch seems like it fixes a clear problem and it's ready to land.
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Did this affect Firefox 17 (and, therefore ESR 17)?
Whiteboard: [adv-main18+]
Group: core-security
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.