766173 - Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager()

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

This isn't security sensitive because this happens before any content is loaded.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 3

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24191df1f866

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Matt Brubeck (:mbrubeck)]

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

Comment 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Mike Hommey [:glandium]]

(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.

Comment 8

[Justin Lebar (not reading bugmail)]

(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...

Comment 9

[Mike Hommey [:glandium]]

(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.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24191df1f866

Comment 11

[Mike Hommey [:glandium]]

(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.

Comment 12

[Justin Lebar (not reading bugmail)]

> 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.

Comment 13

[Mike Hommey [:glandium]]

Door number 4 is using a static variable inside a function. This adds a little overhead, though.

Comment 14

[Justin Lebar (not reading bugmail)]

(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.

Comment 15

[Justin Lebar (not reading bugmail)]

> 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...

Comment 16

[Justin Lebar (not reading bugmail)]

Hopefully this fixes e.g. this crash https://crash-stats.mozilla.com/report/index/054e42ea-e7a2-4dbc-ae17-cafa72120620.

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Justin Lebar (not reading bugmail)]

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.

Comment 19

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da0183b980d9

Comment 20

[Alex Keybl [:akeybl]]

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.

Comment 21

[Ioana (away)]

Is there a way QA can verify this fix?

Comment 22

[Ioana (away)]

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

[Ioana (away)]

marking as fixed per comment 22 (since there were no answers to comment 21).