506348 - Prevent static ns*String instances

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Static string instances can crash XULRunner apps if the instance lives beyond unloading libxul. I added an assertion a long time ago to try and prevent these but a static analysis would be better.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In dehydra, this is a matter of writing a process_decl() function that errors when it sees a decl d for which the following are true

 * d.type.kind == 'class'
 * /ns.*String/.test(d.type.name)
 * d.isStatic

Comment 2

[Benjamin Smedberg]

That's good but not sufficient, because you could have an outer class wrapping a nsString, e.g.

struct Foo {
  nsString bar;
  int i;
};

static Foo foo;

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Good point.

>  * /ns.*String/.test(d.type.name)

This needs a library function to iterate over all "constituent" types of a compound type.  I /think/ this includes (i) recursively, the types all member variables; (ii) recursively, the types of all inherited classes.  I'd call this a "type visitor" if I were working with an object-oriented AST.  Then with typeVisitor, the check is

  typeVisitor(function(t) /ns*String/.test(t.name))

and caching this check could be quite a win.

Comment 4

[Josh Matthews [:jdm]]

Attachment: First stab at catching all static nsString variants

Here's a static analysis I threw together for this.  It seems to work on my very basic test file - any thoughts?

Comment 5

[Josh Matthews [:jdm]]

I did not that any declarations inside of functions don't see to be processed.  Is that a limitation of dehydra, or am I doing something wrong?

Comment 6

[Josh Matthews [:jdm]]

Attachment: Better test for ns*String class name

I tried running this script on mozilla-central, and discovered that the ns*String test needs to be tightened up a bit.  The revised version notes:

/home/jdm/src/firefox/m-c/modules/libpr0n/src/imgLoader.h:226: error: Static instance of class 'imgLoader' contains an ns*String

Since there's a global instance of imgLoader in imgLoader.cpp.  Is this a legitimate problem, or are there some more cases I should be discounting?

Comment 7

[Josh Matthews [:jdm]]

I lied, I read that wrong.  I'm not quite sure what the problem is that's cropping up when running this on imgLoader.cpp.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Analysis with improved string inheritance check

Apologies about the continued review spam.  This version makes it all the way through the tree with no mysterious errors, thanks to humph's input.

Comment 9

[Josh Matthews [:jdm]]

jcranmer pointed out that the [C] should actually be C? in the regex.  I've fixed that locally.

Comment 10

[Josh Matthews [:jdm]]

Attachment: More accurate locations reported with a better sense of where the problem is

I realized that I wasn't using the best loc information when displaying errors, so that's been corrected.  This version also prints out the problematic hierarchy like so:

className.memberType -> problematicBase.memberType

So the |a -> b| shows that b is some problematic base class of a, while a.b shows that b is a problematic member of a.  I'm not sure if there's a better way to display this, but I'm open to suggestions.

Note: this version points out that there's a static imgLoader in imgCacheValidator which contains a string (imgLoader.nsCString) (this all takes place in modules/libpr0n/src).  Is it worth filing a bug about this?

Comment 11

[Josh Matthews [:jdm]]

The libpr0n problem is now in bug 545230.

Comment 12

[Josh Matthews [:jdm]]

Haven't worked on this for years.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add a static analysis to prevent MOZ_NON_STATIC_CLASS classes from being allocated statically

I took a shot at adding the static analysis using clang. The pass appears to work correctly, although it doesn't currently pass on mozilla-central.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add the MOZ_NON_STATIC_CLASS macro to MFBT

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Mark nsTSubstring_CharT as MOZ_NON_STATIC_CLASS

With this change, tests no longer pass.
I switched the analysis to emit a warning, and got the following failures:

../../../../../dist/include/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
../../../../dist/include/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
../../../dist/include/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
../../dist/include/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
../../dist/include/mozilla/dom/PromiseDebugging.h:79:19: warning: variable of type 'nsString' not valid as a static
../dist/include/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/accessible/mac/mozAccessible.mm:582:27: warning: variable of type 'const RoleDescrMap [9]' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/caps/nsPrincipal.cpp:450:33: warning: variable of type 'const nsLiteralCString [58]' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/caps/nsPrincipal.cpp:531:33: warning: variable of type 'const nsLiteralCString [7]' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/dom/devicestorage/nsDeviceStorage.cpp:582:34: warning: variable of type 'const nsLiteralString [4]' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/dom/html/nsHTMLDocument.cpp:1878:34: warning: variable of type 'const nsLiteralString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/dom/media/MediaManager.cpp:823:38: warning: variable of type 'const mozilla::dom::MediaTrackConstraints' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/dom/promise/PromiseDebugging.cpp:90:19: warning: variable of type 'nsString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/dom/promise/PromiseDebugging.h:79:19: warning: variable of type 'nsString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/gfx/layers/FrameMetrics.h:41:29: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/gfx/layers/Layers.cpp:53:34: warning: variable of type 'const mozilla::layers::FrameMetrics' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:189:37: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:190:37: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:191:37: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:2180:37: warning: variable of type 'const nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:251:37: warning: variable of type 'const nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/js/xpconnect/src/XPCJSRuntime.cpp:252:37: warning: variable of type 'const nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/base/ZoomConstraintsClient.cpp:27:30: warning: variable of type 'const nsLiteralString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/base/ZoomConstraintsClient.cpp:28:31: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSKeywords.cpp:83:31: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSProps.cpp:2147:31: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSProps.cpp:2179:29: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSProps.cpp:571:31: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSProps.cpp:583:31: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/layout/style/nsCSSProps.cpp:595:31: warning: variable of type 'nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/security/manager/ssl/tests/gtest/DataStorageTest.cpp:34:26: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/security/manager/ssl/tests/gtest/DataStorageTest.cpp:35:26: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/security/manager/ssl/tests/gtest/DataStorageTest.cpp:36:26: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp:13:26: warning: variable of type 'const nsLiteralCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/toolkit/xre/nsAppRunner.cpp:993:17: warning: variable of type 'nsString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/widget/GfxInfoBase.cpp:1128:24: warning: variable of type 'nsAutoCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/xpcom/string/nsReadableUtils.cpp:1086:34: warning: variable of type 'const nsDependentString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/xpcom/string/nsReadableUtils.cpp:1094:35: warning: variable of type 'const nsDependentCString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/xpcom/string/nsReadableUtils.cpp:1102:30: warning: variable of type 'const nsXPIDLString' not valid as a static
/Volumes/Devel/Code/mozilla/bug506348/xpcom/string/nsReadableUtils.cpp:1110:31: warning: variable of type 'const nsXPIDLCString' not valid as a static

We'll have to fix each of them before this analysis could be merged (or add exceptions to the pass).

Comment 16

[(no longer active)]

Comment on attachment 8627326 [details] [diff] [review]
Part 1: Add a static analysis to prevent MOZ_NON_STATIC_CLASS classes from being allocated statically

Review of attachment 8627326 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +685,5 @@
>    astMatcher.addMatcher(newExpr(hasType(pointerType(
>        pointee(nonheapClassAggregate())
>      ))).bind("node"), &nonheapClassChecker);
> +  // Non-static class assertion: static variables of non-static class are forbidden
> +  astMatcher.addMatcher(varDecl(hasGlobalStorage(),

Looks like this doesn't cover static class members.  I think you need to add a corresponding matcher using fieldDecl as well.

::: build/clang-plugin/tests/TestNonStaticClass.cpp
@@ +1,2 @@
> +#define MOZ_NON_STATIC_CLASS __attribute__((annotate("moz_non_static_class")))
> +#define MOZ_STACK_CLASS __attribute__((annotate("moz_stack_class")))

We don't need to test MOZ_STACK_CLASS stuff here, right?

@@ +28,5 @@
> +  NonStatic valid;
> +  NonStatic alsoValid[2];
> +  SimpleTemplateClass<NonStatic> alsoAlsoValid;
> +  TemplateClass<Stack> stackValid;
> +  static NonStatic notValid; // expected-error {{variable of type 'NonStatic' not valid as a static}}

I would be wary of using "static" in the diagnostics messages here.  This is really about global storage, so I would reword the messages accordingly.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

I'm testing the MOZ_STACK_CLASS stuff here because if a member of a MOZ_NON_STATIC_CLASS is a MOZ_STACK_CLASS, we want to make the class act as a MOZ_STACK_CLASS (as it is more restricting).

With regard to "static", would you like it better if I instead called the annotation MOZ_NON_GLOBAL_CLASS as well?

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Another note: The astMatcher does cover static class members, as those take the form of a varDecl. (fieldDecls only represent values which are attached to instances of a class afaict).

This test expects that a static member of a class which is marked as NonStatic will be rejected:
+struct RandomClass {
+  ...
+  static NonStatic staticMember; // expected-error {{variable of type 'NonStatic' not valid as a static}}
+};

Comment 19

[(no longer active)]

Nathan, is the problem in comment 0 still an issue?  It seems to me that it's not...

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #19)
> Nathan, is the problem in comment 0 still an issue?  It seems to me that
> it's not...

I think the scenario Ben was envisioning goes something like:

- libxul gets loaded
- static constructor for nsString gets run
- destructor for nsString registered to run at program exit
- ...time passes...
- libxul gets unloaded
- program shuts down
- atexit destructor runs, only now refers to garbage

Maybe?  I guess if you don't care about the program shutting down cleanly, it's not a problem...

I don't think that can happen on Linux and OS X, due to using __cxa_at_exit:

http://mentorembedded.github.io/cxx-abi/abi.html#dso-dtor

(3.3.5.1 explicitly describes the problem with library unloading)

Between reading:

https://msdn.microsoft.com/en-us/library/vstudio/988ye33t%28v=vs.100%29.aspx (DLL behavior)
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682583%28v=vs.85%29.aspx (DllMain docs)

I think Windows has the same sort of protections.  All of these mechanisms essentially make the library take responsibility at unload time for any destructors it registered during static construction time.  So I think we're in the clear here, unless I've completely botched Ben's scenario.  Ben?

Comment 21

[(no longer active)]

FWIW, I think there is no value in supporting explicitly unloading libxul.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

A long time ago we definitely hit crashes in XULRunner apps if anyone added a static nsString somewhere in Gecko. Maybe things have changed sufficiently since then? Nathan gets it pretty much right, but I think the crashing part was when the nsString destructor ran it would release its buffer which would then call into the NS_Free functions (or whatever we use for that now) and that function would have already been unloaded.

Comment 23

[Nathan Froyd [:froydnj]]

Is it possible that registering static destructors for libraries on earlier versions of Visual Studio somehow only ran those destructors at program exit, rather than at library unload?  If so, this ought to be a non-issue now, though I think Ehsan's comment 21 is a reasonable stance to take too...

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Marking as WONTFIX (see comment 21)