sixgill annotations for JS

REOPENED
Assigned to

Status

()

Core
JavaScript Engine
--
enhancement
REOPENED
7 years ago
7 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
It would be good to start adding sixgill annotations (http://sixgill.org) to JS.  These annotations fix false positives and make it easy to catch new bugs as they come in.  Reports can be viewed at:

http://dm-sixgill01.mozilla.org

Which is currently behind LDAP (might change soon).  All JS annotations will be attached to this bug.  This attachment does two things:

A) Adds annotation defines to jsutil.h (jsstaticcheck.h isn't included everywhere).  These are also in xpcom/glue/nsDebug.h, which isn't included by JS.  The defines in both headers are modified so that both can be included without issue.

B) Adds 15 annotations to JS for read overflows on Values.  These cut out 571 false positives, mostly related to JS natives and accesses on the JS stack.  There are 48 warnings for such reads left in JS, which need more annotations (will be easier after bug 584917 lands).

Who should review these?
(Assignee)

Comment 1

7 years ago
Created attachment 480113 [details] [diff] [review]
header fixes and annotations
(In reply to comment #0)
> Who should review these?

I heard jorendorff is all about assertions, I'd send it his way. :-)
(Assignee)

Updated

7 years ago
Attachment #480113 - Flags: review?(jorendorff)
Comment on attachment 480113 [details] [diff] [review]
header fixes and annotations

jsutil.h is included from jsapi.h. I don't think we want macro names like
STATIC_ASSERT to be in our public headers. Please put them in jsstaticcheck.h
and include that from jsprvtd.h (which will cover dozens of .cpp files) and
whatever other files need it.

Otherwise this looks good.

>+        STATIC_ASSUME(!p || ubound((char*)p) >= size + incr);              \

Style nit: put a space in "(char *)" please.

>+STATIC_PRECONDITION_ASSUME(ubound(vp) >= argc + 2)
> JS_ALWAYS_INLINE bool
> CallJSNative(JSContext *cx, js::Native native, uintN argc, js::Value *vp)

Wouldn't it be nice if instead of assuming this, we could require the callers
to vouch for it. But OK.

r=me.
Attachment #480113 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

7 years ago
http://hg.mozilla.org/tracemonkey/rev/c55fcc4e8588

I'm hoping that eventually CallJSNative can have a checked precondition, the problem now is that using a checked precondition will just cause the tool to get stuck on complex invariants describing the stack in the interpreter.  The assume vs. assert annotations provide a nice way of marking the boundary between code that can and can't be checked: we can determine the correctness of every JS native, modulo bugs in the interpreter or JITs themselves.  That annotation turned up overflows in the natives, and (provided things are maintained in the future) I'm confident no similar bugs will ship with future versions of Firefox.

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c55fcc4e8588
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

7 years ago
Created attachment 483979 [details] [diff] [review]
annotations

These annotations fix a bunch of warnings in jsemit and jstypedarray, and allow checking for object slot accesses (getSlot, setSlot, etc.)
Assignee: general → bhackett1024
Status: RESOLVED → REOPENED
Attachment #483979 - Flags: review?(jorendorff)
Resolution: FIXED → ---
Attachment #483979 - Flags: review?(jorendorff) → review+

Comment 7

7 years ago
(In reply to comment #6)
> Created attachment 483979 [details] [diff] [review]
> annotations
> 
> These annotations fix a bunch of warnings in jsemit and jstypedarray, and allow
> checking for object slot accesses (getSlot, setSlot, etc.)

Was the annotations patch also landed?
You need to log in before you can comment on or make changes to this bug.