Closed Bug 429031 Opened 16 years ago Closed 16 years ago

Expose more information about precision and signedness via Dehydra Javascript interface


(Developer Infrastructure :: Source Code Analysis, defect)

Not set


(Not tracked)



(Reporter: bk2, Assigned: vladimir.sukhoy)




(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/20080311 Firefox/
Build Identifier: 

GCC provides several important pieces of information which are not exposed for scripting via Javascript in Dehydra:

These fields are vital for static analysis of C++ code for security purposes.  Exposing these fields to Javascript would allow some important questions to be answered in script.

Commonly asked questions during a security code review:
-> Where are all the instances of assigning an unsigned value to a signed type?
   For example:
       unsigned int ui = 0xFFFFFFFF;
       int i = ui;

-> Where are all the instances of truncation (data-loss) via assignment?
   For example:
      int i = 99999;
      short s = i;

Reproducible: Always

Steps to Reproduce:
Ok, let's not touch align and machine modes for now and only do this + unit tests.
Ever confirmed: true
Vlad, I assume you were trying to assign this to yourself?
Assignee: nobody → vladimir.sukhoy
Yes, and messed up at that (urgh, actually this is the second time this precise thing happens with me).
Attached patch wip (obsolete) — Splinter Review
Attached patch better wip (obsolete) — Splinter Review
Now with min and max.
Attachment #315692 - Attachment is obsolete: true
(In reply to comment #3)
> Yes, and messed up at that (urgh, actually this is the second time this precise
> thing happens with me).

Yeah, I was told you can't change the status to assigned and reassign the bug to a person in one step.

Depends on: 429362
What's the status of this work? Ben are you still working on the analysis?

Vlad, the patch looks good. The dehydra_ast part is something that needs to be done better(expr_as_string as it is done currently is a crappy approach). Your changes are an improvement of existing situation.

I think I could be happy with the other changes too, but I need to think about it some more(and test/use cases).
Blocks: 423898
Nevermind the AST part (there's a bug there actually), I think it is worth doing in bug 429446 or something equivalent. Actually I was working on testcases with expr_as_string approach but they were failing because of bug 429362 so I filed bug 429446. Not sure if we want to do testcases with expr_as_string or just go ahead with better representation and do testcases there..
Attached patch patch (obsolete) — Splinter Review
Folded dmandelin's patch from bug 429362 into this, got rid of gcc pretty printer until it is fixed and handled 64-bit ints and type suffixes there, added more constants of various types and added support for precision on pointer/reference, enumeral type and for signedness on enumeral types.
Attachment #315703 - Attachment is obsolete: true
Attached patch patch v2Splinter Review
Noticed a minor glitch in the test which'd break js strict mode, otherwise the same.
Attachment #317330 - Attachment is obsolete: true
Attachment #317336 - Flags: review?(dmandelin)
I really like your testcases, so i was about to commit this, however I'm not sure i like {name:"unsigned char" unsigned:true}
 {unsigned:false, precision:8, name:"signed char"}. This seems redundant. Either the signed/unsigned part should be taken out of the name or it shouldn't be an extra property.

I'm also unsure about unsigned:false, 
a) Dehydra just doesn't include properties when they are false
b) seems kind of backwards, signed:true might work better?

While at it how about prefixing boolean properties with is?

Having said that, perhaps integers are a special enough case to cause rules a) and b) to be bent. Thoughts?
Comment on attachment 317336 [details] [diff] [review]
patch v2

I think isSigned:true/isSigned undefined is a good idea. Getting rid of signedness within .name.. Can be done, but I dunno if it will help or not.
Attachment #317336 - Flags: review?(dmandelin)
it'd have to be isUnsigned i think, since C types default to signed
Vlad, mind if I finish up the details myself?
pushed with is(Uns|S)igned mods
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 431962
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.