Closed Bug 1500247 Opened 2 years ago Closed 2 years ago

HashSet<gcpointer> should be treated as a gcpointer by the hazard analysis

Categories

(Core :: JavaScript: GC, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main67-])

Attachments

(9 files, 6 obsolete files)

1.43 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.71 KB, patch
jonco
: review+
Details | Diff | Splinter Review
9.71 KB, patch
jonco
: review+
Details | Diff | Splinter Review
10.33 KB, patch
jonco
: review+
Details | Diff | Splinter Review
16.72 KB, patch
jonco
: review+
Details | Diff | Splinter Review
5.37 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.54 KB, patch
jonco
: review+
Details | Diff | Splinter Review
16.72 KB, patch
Details | Diff | Splinter Review
18.95 KB, patch
Details | Diff | Splinter Review
From bug 1500064. HashSet and HashMap should be annotated to inherit GC pointeredness from their template parameters.
Group: core-security → javascript-core-security
Keywords: sec-want
Priority: -- → P1
Is this really P1? That is, are we going to work on it during the current cycle (FF66, soft freeze on 21 January)?
Flags: needinfo?(jcoppeard)
(In reply to Jason Orendorff [:jorendorff] from comment #1)
This can hide a potential crashes / security vulnerabilities so I think it should be P1.  Whether Steve has time to work in it by the end of the next train is another matter.
Flags: needinfo?(jcoppeard)
This just worked on try, and found no problems.

Which makes me suspicious. So I am working on a self-test, where I will add uses that *should* fail with the hazard analysis, and add a new annotation mechanism saying "you should get n hazards reported in this source file." That'll require some plumbing, though.

In the meantime, I ran a try build that should fail: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=217062657&revision=5ce94f37e50affb4fc59722cf3a01a450bb4c1df

...and it didn't, so it looks like I have more work to do. Argh.
Ugh. The usual hazard thing. I had a bug where it was only inheriting GC ptrdness from explicitly listed GC pointer types, which are very rare these days, rather than inferred GC pointer types. Which means that the feature wasn't working (for anything but my too-simple test.)

Fixing that bug revealed 12 hazards. 4 are from adding the annotation to GCHash{Map,Set}. 1 of those 4 was an intentionally injected error. That means I have 3 hazards from this bug to fix, plus 8 hazards from the other types that were supposedly getting checked but actually weren't, in this case UniquePtr<T> and Maybe<T>.

Which means I need to fix those and backport them before landing the test. And I really do want to also implement a self-check as part of this. I just can't do it the way I was trying to, which was to add a JS_HAZ_EXPECT_FAILURES(n) that I'd look for whenever detecting a hazard to see if it was expected, because that will not catch the case that actually matters: no hazards detected, expectation of 1 or more.

I'm thinking I'll add an __attribute__ annotation for either the function containing the hazard (which may be inconvenient for jsapi-tests), or for a dummy type that is interpreted at the file scope. I already gather up all attribute values for all types, so that should give me what I need.
Oh yeah, and that's 12 hazards just in the JS shell. I haven't checked the full browser yet. That'll be fun.
Depends on: 1514409
Just an update, since I may have to pause work on this for a bit. The 12 hazards seemed to have 1 true positive, 10 false positives, and 1 intentional error. I have a fix for the real bug, and analysis improvements to remove all but 4 of the false positives. I also have the annotation working to ignore the intentional error, though I just realized that I do *not* have the critical check implemented where it ensures that the error is caught. Oops.

The full browser is showing 11 additional hazards. Most of them appear to be due to WeakCache, which inherits from GCHashMap and so is considered to need rooting. I have an annotation for it so it doesn't need to be rooted (it's swept, not traced, so it should NOT be rooted), but the annotation is processed in the wrong order and so is getting overridden. Still need to fix that.

The other thing I know of that I need to fix before landing this is some hairy UniquePtr handling. I'm still thinking through that one. It's kind of messy.
I was only looking at the specifically labeled GC things, not the inferred ones. Which made the feature do basically nothing.
Attachment #9033610 - Flags: review?(jcoppeard)
Bleh. This requires a number of annotations to get right: the ones for GCHashMap and GCHashSet, plus ones for explicit instantiations of those, but then that makes WeakCache<GCPointer> into something that needs rooting, which it doesn't, so more annotations to turn that off for WeakCache as well as specific instantiations of WeakCache.
Attachment #9033613 - Flags: review?(jcoppeard)
Attachment #9031558 - Attachment is obsolete: true
Use it to verify that MOZ_INHERIT_ATTRIBUTE_FROM_TEMPLATE_PARAM works.

This also cleans up some useless output from the hazard task.
Attachment #9033614 - Flags: review?(jcoppeard)
There are some places where we use a Maybe<AutoCheckCannotGC> to dynamically control whether GC is allowed. Now that the analysis treats Maybe<T> the same as T for the purpose of the hazard analysis, we need to teach the analysis enough data flow to be able to make this work.

Also, this allows setting GC pointers to nullptr and not have the analysis complain about the known constant value living across a GC. (This can be easily extended to NumberValue etc. if needed.)
Attachment #9033615 - Flags: review?(jcoppeard)
{
      extern void foo(UniquePtr<T> u);
      UniquePtr<T> uptr = MakeUnique<T>();
      foo(std::move(uptr));
      gc();
    }

should NOT be a hazard; uptr goes through the move constructor and transfers ownership of the pointer. This is kind of complicated because what actually happens is that std::move() returns a temporary that gets passed to the move constructor, so we have to handle ending the live ranges of both the original UniquePtr as well as the temporary.
Attachment #9033620 - Flags: review?(jcoppeard)
The point of this bug.
Attachment #9033621 - Flags: review?(jcoppeard)
Attachment #9033613 - Attachment is obsolete: true
Attachment #9033613 - Flags: review?(jcoppeard)
Attachment #9033610 - Flags: review?(jcoppeard) → review+
Comment on attachment 9033614 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

This testUnrootedGCHashMap looks pretty unsafe.  Can we test this by adding one of the rootAnalysis/t tests or is this attribute required for other reasons?  I guess it's the intentional error you mentioned.  Could we remove that instead?

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +163,5 @@
>    return true;
>  }
>  END_TEST(testGCRootedHashMap)
>  
> +BEGIN_TEST_WITH_ATTRIBUTES(testUnrootedGCHashMap,JS_EXPECT_HAZARDS) {

Is the idea of this to test the hazard analysis?  This doesn't look very safe.
Attachment #9033614 - Flags: review?(jcoppeard)
Comment on attachment 9033615 [details] [diff] [review]
[hazards] Do not report hazards when a variable is known to hold a safe value

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

This looks fine but can you add a test for this?
Attachment #9033615 - Flags: review?(jcoppeard) → review+
Comment on attachment 9033620 [details] [diff] [review]
[hazards] End the live range of a variable passed to a move constructor via std::move()

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

This looks good but it's non-trivial so I'd like to see some tests.
Attachment #9033620 - Flags: review?(jcoppeard)
Attachment #9033621 - Flags: review?(jcoppeard) → review+
Comment on attachment 9033614 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

Sorry, I should have commented this.

Yes, this is an intentional hazard. The JS_EXPECT_HAZARD annotation will cause the job to fail if the hazard is not detected.

I will add a comment.
But if I write tests, then they will find bugs! Why would I do that?

I did, and they did.
Attachment #9034084 - Flags: review?(jcoppeard)
Attachment #9033615 - Attachment is obsolete: true
More tests, more bugs. Well, missing features this time, I guess.
Attachment #9034085 - Flags: review?(jcoppeard)
Attachment #9033620 - Attachment is obsolete: true
Comment on attachment 9034084 [details] [diff] [review]
[hazards] Do not report hazards when a variable is known to hold a safe value

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

Great, thanks for adding tests.

::: js/src/devtools/rootAnalysis/t/hazards/test.py
@@ +53,5 @@
>  assert('haz8' in hazmap)
> +
> +# safevals hazards. See comments in source.
> +assert('unsafe1' in hazmap)
> +assert('safe2' in hazmap)

Shouldn't this one be not in hazmap as it's safe?
Attachment #9034084 - Flags: review?(jcoppeard) → review+
Comment on attachment 9034085 [details] [diff] [review]
[hazards] End the live range of a variable passed to a move constructor via std::move()

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

Looks good to me.

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +345,5 @@
> +    {
> +        const { Variable: { Name: [ fullname, shortname ] } } = callee;
> +        const [ mangled, unmangled ] = splitFunction(fullname);
> +        // Match a UniquePtr move constructor.
> +        if (unmangled.match(/::UniquePtr<[^>]*>::UniquePtr\((\w+::)*UniquePtr<[^>]*>&&/))

Is it worth factoring out a predicates for things like 'edge is UniquePtr move constructor' and similar?
Attachment #9034085 - Flags: review?(jcoppeard) → review+
For the record, the shell analysis is now green, but the browser analysis has 11 hazards. 8 of them are in generated bindings code -- false positives, but very difficult for the analysis to figure out. Example:

  StringObjectRecordOrLong arg0; // GC pointer
  StringObjectRecordOrLongArgument arg0_holder(arg0); // Conditionally roots, eventually, if needed
  done = (failed = !arg0_holder.TrySetToLong(cx, args[0], tryNext)) || !tryNext; // Can GC
  self->PassUnion22(cx, Constify(arg0)); // Uses arg0

arg0 contains bare pointers. arg0_holder roots it via a Maybe<RecordRooter<...>> that is emplaced as soon as you set the "union" to an object pointer. There's no way for the analysis to work this all out. Hm... though come to think of it, if that last line could pull from arg0_holder so that nothing ever uses arg0, this would probably all just work. That's not possible at the moment, but could be. I don't know what effect it would have on performance or code size.

Otherwise, I'll just need to annotate the heck out of this special case.
Depends on: 1517829
(In reply to Jon Coppeard (:jonco) from comment #19)
> > +# safevals hazards. See comments in source.
> > +assert('unsafe1' in hazmap)
> > +assert('safe2' in hazmap)
> 
> Shouldn't this one be not in hazmap as it's safe?

Er, yes. Yes, it should (not).

Test is valid, code is not. You would think that the name "safe" would imply something to me...
> Test is valid, code is not. You would think that the name "safe" would imply
> something to me...

I just realized I got that wrong too. Test is wrong, code is wrong. Test should say what you said it should, code should implement that. I've fixed it now. (It was checking for 'return nullptr' only, which shows up as '<returnval> = nullptr', when it should have been checking for 'variable = nullptr'.)

Thanks!
This is identical to the previous version of this patch, except for the comment explaining what the test is for. I wasn't totally clear on whether you were ok with this self-test mechanism. I would like to have one, so I can embed tests like this using our actual types and annotations, instead of only testing with unit tests where I fake up the actual types and things.
Attachment #9034481 - Flags: review?(jcoppeard)
Attachment #9033614 - Attachment is obsolete: true
Comment on attachment 9034481 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

Ah I see.  Yes it's good to have end-to-end testing for this.  Thanks.
Attachment #9034481 - Flags: review?(jcoppeard) → review+
Hardcoding it in the analysis script seemed ugly even for me.
Attachment #9035816 - Flags: review?(jcoppeard)
Sorry, this is truly ugly.
Attachment #9035821 - Flags: review?(jcoppeard)
Depends on: 1519284
Argh! I'm an idiot. I just spent a ton of time implementing fancy fixes for what I stupidly took to be a false positive (bug 1519284) but is actually a real hazard.

I guess I'd still like to land this code? You can see the sort of case it handles from the test. It would allow a crazy person to write code like:

    struct Danger {
        Maybe<JSObject*> x;
        Optional<JSObject*> y;
    }

    Danger danger;
    gc();
    if (v.isObject()) {
      danger.x.emplace(&v.toObject());
    }

This patch plus the next will make bug 1519284 report the real hazard instead of getting distracted by one that doesn't actually matter.
Attachment #9035828 - Flags: review?(jcoppeard)
Yet more foolish effort. This patch implements an annotation for types like

    struct BoringDeath {
      Rooted<JSObject> foo;
    };

to say that the destructor never accesses any pointers stored in the instance, so variables' live ranges can be treated as ending earlier and avoiding the bizarro hazard-at-end-of-scope issue that sometimes comes up. You would do it with

    struct BoringDeath {
      Rooted<JSObject> foo;
      ~BoringDeath JS_HAZ_DOES_NOT_USE_THIS {}
    };
Attachment #9035829 - Flags: review?(jcoppeard)
Sorry, I guess I should have implemented the annotation in this patch too.
Attachment #9035830 - Flags: review?(jcoppeard)
Attachment #9035829 - Attachment is obsolete: true
Attachment #9035829 - Flags: review?(jcoppeard)
Attachment #9035816 - Flags: review?(jcoppeard) → review+
Comment on attachment 9035821 [details] [diff] [review]
[hazards] Ignore difficult-to-handle autogenerated DOM bindings code

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

Well this is horrible.  We really need to sort out the rooting situation for these arguments so that we don't need to rely on this.  r+ but only because we have a plan to do that.
Attachment #9035821 - Flags: review?(jcoppeard) → review+
Comment on attachment 9035828 [details] [diff] [review]
[hazards] Annotate and make use of the fact that some classes' zero-arg constructor (eg Maybe<T>) does not populate the instance with any GC pointers, and thus it is safe to GC until something changes

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

This is pretty complicated and I'd rather not add it if it's not actually needed.  Can we talk about this later?
Comment on attachment 9035830 [details] [diff] [review]
[hazards] Implement an annotation to mark that destructors will not use invalidated GC pointers

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

Can we not work this out ourselves?  It strikes me that this annotation would be very easy to get wrong (e.g. if you don't realise that a Heap<T>'s destructor will call the post barrier), or for subsequent changes to invalidate it.
Comment on attachment 9035828 [details] [diff] [review]
[hazards] Annotate and make use of the fact that some classes' zero-arg constructor (eg Maybe<T>) does not populate the instance with any GC pointers, and thus it is safe to GC until something changes

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

Cancelling review for now.
Attachment #9035828 - Flags: review?(jcoppeard)
Attachment #9035830 - Flags: review?(jcoppeard)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Duplicate of this bug: 1257982
Blocks: 1257982
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main67-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.