661567 - WeakMap uses a ContextAllocPolicy, should use a SystemAllocPolicy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Each WeakMap object keeps a pointer to the JSContext in which it was created. It's used to report OOM errors.

Possible consequences:

  * This could cause an OOM error to be reported in the wrong context, which
    is not so bad.

  * This could cause us to try to report OOM in a context that has been
    destroyed, which might be exploitable (we call through a function
    pointer cx->errorReporter).

Comment 1

[Jason Orendorff [:jorendorff]]

I don't know if there are ways for content to cause a cx to be destroyed. If, say, each document gets its own cx then it's probably pretty easy.

I also don't know how easy it is to get an OOM to be reported. Also probably easy on some platforms.

Comment 2

[Andreas Gal :gal]

The lifetime of global objects and contexts matches, and the weak map holds the global object alive. I don't think this can be exploited. Would be good to double check with jst/mrbkap/peterv.

Comment 3

[Andreas Gal :gal]

I am not opposed to the system alloc policy, but we might want to manually report the memory use then so GCs happen (want to make a patch jorendorff?).

Comment 4

[Jim Blandy :jimb]

Attachment: Use ObjectValueMap typedef name where appropriate.

Comment 5

[Jim Blandy :jimb]

Attachment: Use SystemAllocPolicy in JS WeakMap objects, not ContextAllocPolicy.

Comment 6

[Jim Blandy :jimb]

(In reply to comment #2)
> The lifetime of global objects and contexts matches, and the weak map holds
> the global object alive. I don't think this can be exploited. Would be good
> to double check with jst/mrbkap/peterv.

Either way, if there's no need to have the object hold a JSContext reference, that's kind of unusual, and it would be nice to avoid it.

Comment 7

[Luke Wagner [:luke]]

This bug makes a strong argument for taking away the alloc policy of HashMap/Set.

Comment 8

[Jim Blandy :jimb]

I agree.

Comment 9

[Daniel Veditz [:dveditz]]

Assuming sg:critical worst case based on comment 0, please lower if Andreas's comment 2 turns out to be correct.

Comment 10

[Jim Blandy :jimb]

I'll be on PTO Friday, June 3. If the fix looks correct and it needs to land before I'm back on Monday, please feel free to take care of it.

Comment 11

[Jim Blandy :jimb]

Comment on attachment 536932 [details] [diff] [review]
Use ObjectValueMap typedef name where appropriate.

Hi, Andreas. Jason is out for the week; could you review this?

Comment 12

[Andreas Gal :gal]

Comment on attachment 536933 [details] [diff] [review]
Use SystemAllocPolicy in JS WeakMap objects, not ContextAllocPolicy.

Please add proper malloc bytes accounting for the current cx, with that r=me.

Comment 13

[Jim Blandy :jimb]

(In reply to comment #7)
> This bug makes a strong argument for taking away the alloc policy of
> HashMap/Set.

Or possibly it makes an argument for having 'add' and anything else that can allocate take a policy-related argument as well. (Sounds like a pain.)

Comment 14

[Jim Blandy :jimb]

Attachment: Implement RuntimeAllocPolicy, providing proper memory accounting in GC'd objects that live longer than a JSContext.

Comment 15

[Jim Blandy :jimb]

Attachment: Use RuntimeAllocPolicy in JS WeakMap objects, not ContextAllocPolicy.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 537604 [details] [diff] [review]
Implement RuntimeAllocPolicy, providing proper memory accounting in GC'd objects that live longer than a JSContext.

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

::: js/src/jscntxt.h
@@ +1440,5 @@
> + * Since it doesn't hold a JSContext (those may not live long enough), it
> + * can't report out-of-memory conditions itself; the caller must check for
> + * OOM and take the appropriate action.
> + */
> +class RuntimeAllocPolicy

Can you put this decl in jsalloc.h next to ContextAllocPolicy?

@@ +1448,5 @@
> +    /*
> +     * Non-inline helper to call JSRuntime::onOutOfMemory with minimal
> +     * code bloat.
> +     */
> +    JS_FRIEND_API(void *) onOutOfMemory(void *p, size_t nbytes);

I think this is a dead decl.

Comment 17

[Jim Blandy :jimb]

(In reply to comment #16)
> Comment on attachment 537604 [details] [diff] [review] [review]
> Implement RuntimeAllocPolicy, providing proper memory accounting in GC'd
> objects that live longer than a JSContext.
> 
> Review of attachment 537604 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jscntxt.h
> @@ +1440,5 @@
> > + * Since it doesn't hold a JSContext (those may not live long enough), it
> > + * can't report out-of-memory conditions itself; the caller must check for
> > + * OOM and take the appropriate action.
> > + */
> > +class RuntimeAllocPolicy
> 
> Can you put this decl in jsalloc.h next to ContextAllocPolicy?

I tried that first, but I'd like to actually use the context in the inline functions. Is it okay to #include "jscntxt.h" in "jsalloc.h"? I had the impression it was supposed to be a lighter header than that.

> 
> @@ +1448,5 @@
> > +    /*
> > +     * Non-inline helper to call JSRuntime::onOutOfMemory with minimal
> > +     * code bloat.
> > +     */
> > +    JS_FRIEND_API(void *) onOutOfMemory(void *p, size_t nbytes);
> 
> I think this is a dead decl.

Sloppy. Thanks, fixed.

Comment 18

[Luke Wagner [:luke]]

(In reply to comment #17)
> > Can you put this decl in jsalloc.h next to ContextAllocPolicy?
> 
> I tried that first, but I'd like to actually use the context in the inline
> functions. Is it okay to #include "jscntxt.h" in "jsalloc.h"? I had the
> impression it was supposed to be a lighter header than that.

Ah, of course; jscntxt.h seems fine.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #3)
> I am not opposed to the system alloc policy, but we might want to manually
> report the memory use then so GCs happen (want to make a patch jorendorff?).

No JS-heap-entangled data structure in the core engine should keep a JSContext pointer. nsJSContext in the DOM does but that's an exception to the rule even if you consider it core.

We're moving JSContext (stack of 'em I guess) to TLS, I hear, so we can get away from the general problem of needing a cx and not having one. We'll have the wrong cx (dynamic scope) problem, though, which can be really bad. I've discussed this briefly with dmandelin.

/be

Comment 20

[Jim Blandy :jimb]

(In reply to comment #19)
> No JS-heap-entangled data structure in the core engine should keep a
> JSContext pointer. nsJSContext in the DOM does but that's an exception to
> the rule even if you consider it core.

Yeah. The present patch holds no JSContext pointers; instead, users of the RuntimeAllocPolicy hold a pointer to their Runtime, which seems fine for JavaScript objects.

Comment 21

[LegNeato]

Would it be ok to ship Fx5 with this issue or is it bad enough we either need to wait for this fix or back out / turn off WeakMap?

Comment 22

[Jim Blandy :jimb]

Nobody knows of any exploits. It's just bad design, as far as we know.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 537605 [details] [diff] [review]
Use RuntimeAllocPolicy in JS WeakMap objects, not ContextAllocPolicy.

r=me.

Andreas asked about adding cx-accounting for mallocs under WeakMap operations, but it seems like a totally orthogonal concern. Follow-up bug, Andreas?

Comment 24

[Jim Blandy :jimb]

(In reply to comment #23)
> Andreas asked about adding cx-accounting for mallocs under WeakMap
> operations, but it seems like a totally orthogonal concern. Follow-up bug,
> Andreas?

The RuntimeAllocPolicy does all the accounting Andreas was hoping for, so this should be okay.

Comment 25

[Jason Orendorff [:jorendorff]]

Yes. Right. Carry on.

Comment 26

[Andreas Gal :gal]

This code is racy with workers, but we are about to make those 1-per-runtime so I think thats ok.

Comment 27

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/666e14265c6b

Comment 28

[Jim Blandy :jimb]

I'm not sure what to do with the testcase-wanted. We don't know any exploits, which means we don't know how to provoke the erroneous behavior externally.

Comment 29

[Jason Orendorff [:jorendorff]]

jimb did this:

           What    |Removed                     |Added
----------------------------------------------------------------------------
    status-firefox7|affected                    |---
    status-firefox6|affected                    |---
  tracking-firefox5|-                           |?
        status1.9.1|unaffected                  |---
  tracking-firefox6|+                           |---
        status1.9.2|unaffected                  |---
  tracking-firefox7|+                           |---
  Status Whiteboard|[sg:critical?]              |[sg:critical?],
                   |                            |fixed-in-tracemonkey
    status-firefox5|wontfix                     |---

I don't think he intended most of that; now idea how or why that happens.

Removing testcase-wanted (intentionally) because I don't think we want one bad enough for people to spend time on it.

Comment 30

[Jason Orendorff [:jorendorff]]

(In reply to comment #26)
> This code is racy with workers, but we are about to make those 1-per-runtime
> so I think thats ok.

You mean the malloc accounting races? I think that's the case already with malloc accounting generally, whether it's cx-based or rt-based. There's a comment:

    void updateMallocCounter(size_t nbytes) {
        /* We tolerate any thread races when updating gcMallocBytes. */
        ...

Comment 31

[Daniel Veditz [:dveditz]]

Since we don't know how to demonstrate even symptoms of this problem it's hard to justify an sg:critical rating.

Comment 32

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/fda88360e14f
http://hg.mozilla.org/mozilla-central/rev/89c54bd5bcb2
http://hg.mozilla.org/mozilla-central/rev/666e14265c6b

Comment 33

[Asa Dotzler [:asa]]

This bug is currently tracking-firefox6+  Are the patches here candidates for Firefox 6 Beta? (well tested, low risk, safe :)

Comment 34

[Andreas Gal :gal]

Pretty trivial patch. Unlikely to break anything.

Comment 35

[LegNeato]

sg-moderate seems not worth it, we'll wait for the next go round.

Comment 36

[Daniel Veditz [:dveditz]]

(In reply to comment #29)
> I don't think he intended most of that; no idea how or why that happens.

If someone changes the bug while you've got it open normally you will get a "mid-air collision" warning. If, however, you RELOAD after the other person changes the bug then there will be no warning, but the state of the fields in your bug will persist and eventually overwrite the changes. This happens in practice when people have bugs open in long-lived tabs.

To avoid this problem you must SHIFT-RELOAD. People are reluctant to do that if they've made edits since that will undo the edits they wanted in addition to resetting the other fields. If you RELOAD without doing a SHIFT-RELOAD you are defeating the mid-air-warning protection and MUST check the bug history after you submit to make sure you didn't change things you didn't intend. The "Bugzilla Tweaks" add-on makes this a little easier: those changes will be displayed in-line so you don't have to click the "History" link in the bug.

Changing gears, this landed on m-c on 6/20 so it's fixed in Fx7: updating flags to reflect that. "wontfix" in Fx6.

Comment 37

[u279076]

qa-: no QA verification necessary