Closed Bug 661567 Opened 13 years ago Closed 13 years ago

WeakMap uses a ContextAllocPolicy, should use a SystemAllocPolicy

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7
Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 + fixed
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jorendorff, Assigned: jimb)

Details

(Whiteboard: [sg:moderate], [landed m-c 6/20] fixed-in-tracemonkey [qa-])

Attachments

(3 files, 1 obsolete file)

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).
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.
Assignee: general → jimb
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.
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?).
Attachment #536932 - Flags: review?(jorendorff)
Attachment #536933 - Flags: review? → review?(jorendorff)
(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.
This bug makes a strong argument for taking away the alloc policy of HashMap/Set.
I agree.
Assuming sg:critical worst case based on comment 0, please lower if Andreas's comment 2 turns out to be correct.
Keywords: testcase-wanted
Whiteboard: [sg:critical?]
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 on attachment 536932 [details] [diff] [review]
Use ObjectValueMap typedef name where appropriate.

Hi, Andreas. Jason is out for the week; could you review this?
Attachment #536932 - Flags: review?(jorendorff) → review?(gal)
Attachment #536933 - Flags: review?(jorendorff) → review?(gal)
Attachment #536932 - Flags: review?(gal) → review+
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.
Attachment #536933 - Flags: review?(gal) → review+
(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.)
Attachment #536933 - Attachment is obsolete: true
Attachment #537605 - Flags: review?(gal)
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.
(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.
(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.
Attachment #537604 - Flags: review?(luke) → review+
(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
(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.
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?
Nobody knows of any exploits. It's just bad design, as far as we know.
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?
Attachment #537605 - Flags: review?(gal) → review+
(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.
Yes. Right. Carry on.
This code is racy with workers, but we are about to make those 1-per-runtime so I think thats ok.
http://hg.mozilla.org/tracemonkey/rev/666e14265c6b
Whiteboard: [sg:critical?] → [sg:critical?], fixed-in-tracemonkey
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.
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.
Keywords: testcase-wanted
(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. */
        ...
Since we don't know how to demonstrate even symptoms of this problem it's hard to justify an sg:critical rating.
Whiteboard: [sg:critical?], fixed-in-tracemonkey → [sg:moderate], fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This bug is currently tracking-firefox6+  Are the patches here candidates for Firefox 6 Beta? (well tested, low risk, safe :)
Whiteboard: [sg:moderate], fixed-in-tracemonkey → [sg:moderate], [landed m-c 6/20] fixed-in-tracemonkey
Pretty trivial patch. Unlikely to break anything.
sg-moderate seems not worth it, we'll wait for the next go round.
(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.
Target Milestone: --- → mozilla7
qa-: no QA verification necessary
Whiteboard: [sg:moderate], [landed m-c 6/20] fixed-in-tracemonkey → [sg:moderate], [landed m-c 6/20] fixed-in-tracemonkey [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.