Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager()

RESOLVED FIXED in Firefox 15

Status

()

Core
Security: CAPS
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug, {crash, qawanted})

Trunk
mozilla16
All
Windows 7
crash, qawanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 affected, firefox14 affected, firefox15 verified, firefox16 fixed)

Details

(Whiteboard: [startupcrash], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
nsScriptSecurityManager* ssManager = new nsScriptSecurityManager();
        if (!ssManager)
            return nsnull;
        nsresult rv;
        rv = ssManager->Init();
        if (NS_FAILED(rv)) {
            delete ssManager;
            return nsnull;
        }

We're screwed if Init() addrefs and releases ssManager.  We're particularly screwed if Init() then returns false.

From bug 764192 comment 24, it appears that we're hitting this delete and dying.  This may or may not be the cause of bug 709860.
(Assignee)

Comment 1

5 years ago
This isn't security sensitive because this happens before any content is loaded.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 2

5 years ago
Created attachment 634484 [details] [diff] [review]
Patch v1
Attachment #634484 - Flags: review?(benjamin)

Updated

5 years ago
Blocks: 709860

Updated

5 years ago
Attachment #634484 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/24191df1f866
(Assignee)

Comment 4

5 years ago
Comment on attachment 634484 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Before CVS --> hg transition
User impact if declined: Startup crash
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Could somehow change behavior here, but this is a likely improvement over what was there.
String or UUID changes made by this patch: None.
Attachment #634484 - Flags: approval-mozilla-aurora?
This patch added one static initializer, according to Number of Constructor benchmark:
http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1340121005773.153,1340131255096.7742&displayrange=7&datatype=running
(Assignee)

Comment 6

5 years ago
I've been operating under the assumption that these will be optimized out by any not-brain-dead compiler.  The initializer does nothing more than set a variable in the data section to 0, but the data section is already initialized to 0.

If our compilers really can't optimize this, we can make versions of ns{COM,Ref}Ptr that don't initialize themselves to 0.  But that's really a separate issue from this patch; the idiom of using a static smart pointer appears basically everywhere that ClearOnShutdown is used.
(In reply to Justin Lebar [:jlebar] from comment #6)
> I've been operating under the assumption that these will be optimized out by
> any not-brain-dead compiler.

gcc is brain-dead.
(Assignee)

Comment 8

5 years ago
(In reply to Mike Hommey [:glandium] from comment #7)
> gcc is brain-dead.

:(

How much do we care about this extra static initializer?  I guess "some" because we use gcc on mobile.  I can't say fixing this is high on my list of priorities, though...
(In reply to Justin Lebar [:jlebar] from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #7)
> > gcc is brain-dead.
> 
> :(
> 
> How much do we care about this extra static initializer?  I guess "some"
> because we use gcc on mobile.  I can't say fixing this is high on my list of
> priorities, though...

I'll post something on dev-platform, because this is one of the typical cases (meaning there are several such cases to fix), and that has different possible solutions, one of which I find a elegant hack but Waldo disagrees.
https://hg.mozilla.org/mozilla-central/rev/24191df1f866
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(In reply to Mike Hommey [:glandium] from comment #9)
> I'll post something on dev-platform, because this is one of the typical
> cases (meaning there are several such cases to fix), and that has different
> possible solutions, one of which I find a elegant hack but Waldo disagrees.

Actually, it's maybe not that common a pattern to grant a generalized hack. In this particular case, it shouldn't be hard to make the variable non-global.
(Assignee)

Comment 12

5 years ago
> Actually, it's maybe not that common a pattern

$ git ls-files | grep -v "ClearOnShutdown" | xargs grep 'ClearOnShutdown('
dom/base/Navigator.cpp:    ClearOnShutdown(&gVibrateWindowListener);
dom/power/PowerManagerService.cpp:    ClearOnShutdown(&sSingleton);
hal/Hal.cpp:  ClearOnShutdown(&gLastIDToVibrate);
hal/HalWakeLock.cpp:  ClearOnShutdown(&sLockTable);
image/src/RasterImage.cpp:    ClearOnShutdown(&sSingleton);
widget/gonk/OrientationObserver.cpp:    ClearOnShutdown(&sOrientationSensorObserver);
(plus this patch, which is not in my tree)

All of these have global (or class-static) smart pointers.

> In this particular case, it shouldn't be hard to make the variable non-global.

The variable needs to be, effectively, a global strong reference.  So we can either actually make it a global strong reference, or alternatively we can make a weak reference to an object on the heap which keeps a strong reference, and then register a shutdown observer to free the object.

I guess door number 3 is, use a raw pointer and manually addref/release.  In at least one instance above, the value stored in the global changes, so this would be particularly error-prone.
Door number 4 is using a static variable inside a function. This adds a little overhead, though.
(Assignee)

Comment 14

5 years ago
(In reply to Mike Hommey [:glandium] from comment #13)
> Door number 4 is using a static variable inside a function.

The initializer is not run until the first time the function is run, I guess?  That could work.  I've never seen this used in hot code.
(Assignee)

Comment 15

5 years ago
> Door number 4 is using a static variable inside a function.

This doesn't work in the case when we need to be able to change the variable, for example gLastIDToVibrate.  I mean, you could have a method with a bunch of parameters which either gets or sets the variable, but that's pretty ugly...
(Assignee)

Updated

5 years ago
Summary: Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager() → Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager(), likely responsible for crash [@ arena_dalloc_small | XPCJSContextStack::GetSafeJSContext() ]
(Assignee)

Comment 16

5 years ago
Hopefully this fixes e.g. this crash https://crash-stats.mozilla.com/report/index/054e42ea-e7a2-4dbc-ae17-cafa72120620.
(Assignee)

Updated

5 years ago
Crash Signature: [@ arena_dalloc_small | XPCJSContextStack::GetSafeJSContext()]
Summary: Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager(), likely responsible for crash [@ arena_dalloc_small | XPCJSContextStack::GetSafeJSContext() ] → Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager()
(Assignee)

Updated

5 years ago
Keywords: qawanted
Comment on attachment 634484 [details] [diff] [review]
Patch v1

[Triage Comment]
No obvious regressions noted in today's build, so let's land on Aurora 15.
Attachment #634484 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
Comment on attachment 634484 [details] [diff] [review]
Patch v1

[Approval Request Comment]
I'm not sure if we should take this on beta, but it's worth considering.  Perhaps we can see how the crash rate on Aurora is affected by its landing.
Attachment #634484 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/da0183b980d9
(Assignee)

Updated

5 years ago
Keywords: crash
(Assignee)

Updated

5 years ago
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
status-firefox16: --- → fixed

Updated

5 years ago
Severity: normal → critical
OS: Linux → Windows 7
Hardware: x86_64 → All
Whiteboard: [startupcrash]

Updated

5 years ago
Blocks: 718575
Comment on attachment 634484 [details] [diff] [review]
Patch v1

Given https://bugzilla.mozilla.org/show_bug.cgi?id=709860#c39 I don't think we should land on Beta.
Attachment #634484 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Comment 21

5 years ago
Is there a way QA can verify this fix?

Comment 22

5 years ago
The crash from comment 16 doesn't reproduce anymore according to the Socorro interface:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=arena_dalloc_small%20%7C%20XPCJSContextStack%3A%3AGetSafeJSContext%28%29

Comment 23

5 years ago
marking as fixed per comment 22 (since there were no answers to comment 21).
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.