Closed Bug 1037889 Opened 6 years ago Closed 6 years ago

CID 1225404: Bad bit shift operation (BAD_SHIFT) as found by Coverity

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 --- unaffected
firefox32 --- affected
firefox33 --- affected

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, regression)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #991016 +++

Coverity analysis of source code in js/src has found a Bad bit shift operation, that probably happened recently.


________________________________________________________________________________________________________
*** CID 1225404:  Bad bit shift operation  (BAD_SHIFT)
/js/src/jsnum.cpp: 1759 in js_strtod<unsigned char>(js::ThreadSafeContext *, const T1 *, const T1 *, const T1 **, double *)()
1753         Vector<char, 32> chars(cx);
1754         if (!chars.growByUninitialized(length + 1))
1755             return false;
1756     
1757         size_t i = 0;
1758         for (; i < length; i++) {
>>>     CID 1225404:  Bad bit shift operation  (BAD_SHIFT)
>>>     In expression "s[i] >> 8", right shifting "s[i]" by more than 7 bits always yields zero.  The shift amount is 8.
1759             if (s[i] >> 8)
1760                 break;
1761             chars[i] = char(s[i]);
1762         }
1763         chars[i] = 0;
1764     

The line was added in: https://hg.mozilla.org/mozilla-central/rev/1962fd3aa819 in a changeset by Jan.

Jan, any thoughts on how to move forward here? (not sure how bad this is, so setting s-s first.)
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jan, any thoughts on how to move forward here? (not sure how bad this is, so
> setting s-s first.)

This is harmless. Latin1Char is 8-bit so "s[i] >> 8" is indeed always 0 but that's fine. This function is templated though so we can't remove it: if CharT is jschar the result can be != 0.

Gary, is it easy for you to check if changing:

if (s[i] >> 8)

to:

if (jschar(s[i]) >> 8)

helps?
Flags: needinfo?(gary)
Nope, it is not easy for me to do it. dveditz runs occasional (monthly?) Coverity runs and I (even more) occasionally take a look at the results.

Harmless -> opening up.
Group: core-security, javascript-core-security
Flags: needinfo?(gary)
Gary, it is useful to put something like [cid 1234] for Coverity bugs, so we can find the report from the bugzilla bug.  And of course, put a link to this bug in the Coverity report.
Oops, never mind, you put it in the name of the bug itself.  Great. :)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> And of course, put a link to this bug in the Coverity report.

Yes, I also did this.
Attached patch WorkaroundSplinter Review
This should silence Coverity. We use jschar but with some basic range analysis the compiler should be able to eliminate the "if (c >> 8)" anyway if CharT is Latin1Char.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8465333 - Flags: review?(benj)
Flags: needinfo?(jdemooij)
Comment on attachment 8465333 [details] [diff] [review]
Workaround

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

LGTM
Attachment #8465333 - Flags: review?(benj) → review+
(In reply to Jan de Mooij [:jandem] from comment #6)
> This should silence Coverity. We use jschar but with some basic range
> analysis the compiler should be able to eliminate the "if (c >> 8)" anyway
> if CharT is Latin1Char.

Alternate solution is to say |if ((c >> 4) >> 4)|
https://hg.mozilla.org/mozilla-central/rev/33654f4fe547
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.