Last Comment Bug 766173 - Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager()
: Potential malloc error in nsScriptSecurityManager::GetScriptSecurityManager()
Status: RESOLVED FIXED
[startupcrash]
: crash, qawanted
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All Windows 7
: -- critical (vote)
: mozilla16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: jemalloc-assertions
Blocks: 718575 709860
  Show dependency treegraph
 
Reported: 2012-06-19 09:53 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-08-28 01:31 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
verified
fixed


Attachments
Patch v1 (3.15 KB, patch)
2012-06-19 10:06 PDT, Justin Lebar (not reading bugmail)
benjamin: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-06-19 09:53:39 PDT
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) 2012-06-19 09:53:58 PDT
This isn't security sensitive because this happens before any content is loaded.
Comment 2 Justin Lebar (not reading bugmail) 2012-06-19 10:06:06 PDT
Created attachment 634484 [details] [diff] [review]
Patch v1
Comment 3 Justin Lebar (not reading bugmail) 2012-06-19 10:21:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/24191df1f866
Comment 4 Justin Lebar (not reading bugmail) 2012-06-19 12:42:28 PDT
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) 2012-06-19 12:45:38 PDT
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) 2012-06-19 12:52:23 PDT
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] 2012-06-19 13:33:19 PDT
(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) 2012-06-19 16:48:30 PDT
(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] 2012-06-19 22:50:00 PDT
(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] 2012-06-20 02:21:05 PDT
https://hg.mozilla.org/mozilla-central/rev/24191df1f866
Comment 11 Mike Hommey [:glandium] 2012-06-20 05:30:57 PDT
(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) 2012-06-20 07:10:15 PDT
> 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] 2012-06-20 07:45:07 PDT
Door number 4 is using a static variable inside a function. This adds a little overhead, though.
Comment 14 Justin Lebar (not reading bugmail) 2012-06-20 07:49:19 PDT
(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) 2012-06-20 08:04:27 PDT
> 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) 2012-06-20 14:05:19 PDT
Hopefully this fixes e.g. this crash https://crash-stats.mozilla.com/report/index/054e42ea-e7a2-4dbc-ae17-cafa72120620.
Comment 17 Alex Keybl [:akeybl] 2012-06-20 15:29:43 PDT
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) 2012-06-20 20:03:03 PDT
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) 2012-06-20 20:15:16 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/da0183b980d9
Comment 20 Alex Keybl [:akeybl] 2012-06-24 12:21:22 PDT
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) 2012-08-10 06:09:39 PDT
Is there a way QA can verify this fix?
Comment 23 Ioana (away) 2012-08-28 01:31:40 PDT
marking as fixed per comment 22 (since there were no answers to comment 21).

Note You need to log in before you can comment on or make changes to this bug.