Expose more information about precision and signedness via Dehydra Javascript interface

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
7 months ago

People

(Reporter: bk2, Assigned: vladimir.sukhoy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Ok, let's not touch align and machine modes for now and only do this + unit tests.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Vlad, I assume you were trying to assign this to yourself?
Assignee: nobody → vladimir.sukhoy
Status: ASSIGNED → NEW
(Assignee)

Comment 3

11 years ago
Yes, and messed up at that (urgh, actually this is the second time this precise thing happens with me).
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 years ago
Created attachment 315692 [details] [diff] [review]
wip
(Assignee)

Comment 5

11 years ago
Created attachment 315703 [details] [diff] [review]
better wip

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.

(Assignee)

Updated

11 years ago
Depends on: 429362

Comment 7

11 years ago
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
(Assignee)

Comment 8

11 years ago
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..
(Assignee)

Comment 9

11 years ago
Created attachment 317330 [details] [diff] [review]
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.
Attachment #315703 - Attachment is obsolete: true
(Assignee)

Comment 10

11 years ago
Created attachment 317336 [details] [diff] [review]
patch v2

Noticed a minor glitch in the test which'd break js strict mode, otherwise the same.
Attachment #317330 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #317336 - Flags: review?(dmandelin)

Comment 11

11 years ago
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?
(Assignee)

Comment 12

11 years ago
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)

Comment 13

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

Comment 14

11 years ago
Vlad, mind if I finish up the details myself?

Comment 15

11 years ago
pushed with is(Uns|S)igned mods
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Depends on: 431962

Updated

7 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.