429031 - Expose more information about precision and signedness via Dehydra Javascript interface

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Ben Kurtz]

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


GCC provides several important pieces of information which are not exposed for scripting via Javascript in Dehydra:
1) TYPE_MIN_VALUE
2) TYPE_MAX_VALUE
3) TYPE_PRECISION
4) TYPE_UNSIGNED

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:
1.
2.
3.

Comment 1

[Vlad Sukhoy]

Ok, let's not touch align and machine modes for now and only do this + unit tests.

Comment 2

[David Mandelin [:dmandelin]]

Vlad, I assume you were trying to assign this to yourself?

Comment 3

[Vlad Sukhoy]

Yes, and messed up at that (urgh, actually this is the second time this precise thing happens with me).

Comment 4

[Vlad Sukhoy]

Attachment: wip

Comment 5

[Vlad Sukhoy]

Attachment: better wip

Now with min and max.

Comment 6

[David Mandelin [:dmandelin]]

(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.

Comment 7

[(dormant account)]

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).

Comment 8

[Vlad Sukhoy]

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..

Comment 9

[Vlad Sukhoy]

Attachment: patch

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.

Comment 10

[Vlad Sukhoy]

Attachment: patch v2

Noticed a minor glitch in the test which'd break js strict mode, otherwise the same.

Comment 11

[(dormant account)]

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 12

[Vlad Sukhoy]

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.

Comment 13

[(dormant account)]

it'd have to be isUnsigned i think, since C types default to signed

Comment 14

[(dormant account)]

Vlad, mind if I finish up the details myself?

Comment 15

[(dormant account)]

pushed with is(Uns|S)igned mods