Handle Arrays in the analysis

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks: 1 bug)

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Value[] foo = { ... };

is currently being entirely ignored, because it shows up in the analysis as an "Array", and the analysis only knows "Pointer" and "CSU" (Class/Struct/Union.)
(Assignee)

Comment 1

4 years ago
Created attachment 8365276 [details] [diff] [review]
Handle Arrays in the analysis

Large bump in the number of hazards.
Attachment #8365276 - Flags: review?(terrence)
(Assignee)

Updated

4 years ago
Group: core-security
Comment on attachment 8365276 [details] [diff] [review]
Handle Arrays in the analysis

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

r=me
Attachment #8365276 - Flags: review?(terrence) → review+
(Assignee)

Updated

4 years ago
Blocks: 898606
I've verified that all identified hazards are false positives or in tests, so opening up.
Group: core-security
Created attachment 8366919 [details] [diff] [review]
hazards_array-v0.diff

This is all the low-hanging fruit. I have one other largish patch for nsJSNPRuntime, which I've split off for gecko peer review. There are three remaining hazards which are all false positives for which simple code reorganization will not work. We'll need to think of some other mechanism(s) for those.
Attachment #8366919 - Flags: review?(jcoppeard)
Try run for the stack:
https://tbpl.mozilla.org/?tree=Try&rev=f29bb9b0d9b6
Created attachment 8366950 [details] [diff] [review]
hazard_npapi-v0.diff

Will flag this for review when the try run is green.
Comment on attachment 8366950 [details] [diff] [review]
hazard_npapi-v0.diff

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ -634,5 @@
>    } else {
>      fv = OBJECT_TO_JSVAL(npjsobj->mJSObj);
>    }
>  
> -  JS::Value jsargs_buf[8];

Could we use AutoValueVector here?  It looks like it will handle allocating the elements possibly-inline for us.
Comment on attachment 8366919 [details] [diff] [review]
hazards_array-v0.diff

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

This all looks good.
Attachment #8366919 - Flags: review?(jcoppeard) → review+
I noticed the try run was failing.  It turns out this was caused by the fix for bug 962256, so I kicked off a new one without that patch:

https://tbpl.mozilla.org/?tree=Try&rev=916abaa43b1f
Try looks good:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4d2dd81858
Created attachment 8367698 [details] [diff] [review]
hazard_npapi-v1.diff

Great idea, Jon! The default inline count is even 8, so we don't even need to pass through new template args to get identical behavior.
Attachment #8367698 - Flags: review?(benjamin)
Created attachment 8367709 [details] [diff] [review]
hazard_npapi-v1.diff

This time with more qreffing.
Attachment #8366950 - Attachment is obsolete: true
Attachment #8367698 - Attachment is obsolete: true
Attachment #8367698 - Flags: review?(benjamin)
Attachment #8367709 - Flags: review?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/ce4d2dd81858
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]

Updated

4 years ago
Attachment #8367709 - Flags: review?(benjamin) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7400843fb1c0
https://hg.mozilla.org/mozilla-central/rev/7400843fb1c0
And we're good now.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1143786
(Assignee)

Comment 17

3 years ago
Whoops, this should never have been closed. The actual hazards have been fixed, but the analysis patch never landed because there were some nuisance false positives left.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

3 years ago
Created attachment 8585561 [details] [diff] [review]
Assume(ptr) is not a hazard; Assume(*ptr) still is

When adding in stack arrays to the analysis, I hit a false positive that looked
something like this:

  GCPointer localArray[10];
  GCPointer *data = nullptr;
  localArray[3] = JS::NewGCThing(...);
  if (...)
    data = localArray;
  canGC();
  if (data != localArray)
    free(data);

The idea is that you get a fixed-size buffer on the stack to store a set of GC pointers, and if you need more space it gets malloced. The analysis sees this as a hazard, because 'localArray' is live after the canGC() call due to its use in the |if (data != localArray)| call. But its *contents* are never examined, so this is a false positive -- if a GC thing in localArray were moved, nothing would ever see it.

So I'd like to allow this use by saying that Assume(...var...) doesn't extend var's live range, unless it's in the form of Assume(...*var...).

I believe that this might also help with situations like:

  JSObject *raw = ...;
  RootedObject rooted(cx, raw);
  canGC();
  if (rooted != raw) {
    ...do expensive fixup stuff...
  }

though perhaps the fixup stuff will always end up using 'raw' in a way that extends the live range? I haven't looked at examples. (The attached patch does not ignore any Assign or Call that uses the variable without dereferencing it.)
Attachment #8585561 - Flags: review?(bhackett1024)
Attachment #8585561 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 19

3 years ago
Created attachment 8588745 [details] [diff] [review]
Annotate XPCNativeMember

Ok, now that I'm all educated on interning (pinning) and understand that this is not a hazard because of the interning (pinning), I agree that this should just be annotated away.

The most precise annotation I could come up with is for XPCNativeMember (since I didn't want to annotate away jsid entirely, even though an argument could be made for it.)

But it's a weird annotation. And I hope to stop pinning these jsids, in which case we'll need to root here after all. But that will also require fixing NPIdentifier, so I stuck in an annotation for it at the same time. (The analysis currently doesn't see it because the compiler only sees it as a void*.) That way, I'll remember to remove both annotations at the same time.
Attachment #8588745 - Flags: review?(terrence)
Attachment #8588745 - Flags: review?(terrence) → review+
(Assignee)

Updated

3 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b471ab566d1c
https://hg.mozilla.org/mozilla-central/rev/be2e7aeee256
https://hg.mozilla.org/mozilla-central/rev/3ac7d11365a2
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.