[Static Analysis][Operands don't affect result] In functions js::RegExpMatcherRaw() and js::RegExpTesterRaw()

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla47
coverity
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

(Whiteboard: CID 1347680, CID 1347681)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
The Static Analysis tool Coverity added that below code from functions js::RegExpMatcherRaw() and js::RegExpTesterRaw() don't affect the outcome of the flow:

>>    MOZ_ASSERT(lastIndex <= INT32_MAX);

Since in both cases type of lastIndex is int32_t and INT32_MAX is:

>>   #define INT32_MAX        2147483647i32 

MOZ_ASSERT becomes useless. Although code is used only on debug we should still remove it.
(Assignee)

Comment 1

2 years ago
Created attachment 8707389 [details]
MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

Review commit: https://reviewboard.mozilla.org/r/30673/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30673/
Attachment #8707389 - Flags: review?(jorendorff)
Maybe it would make sense to change this assertion to test that lastIndex does not go above the Max string length?
(Assignee)

Comment 3

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Maybe it would make sense to change this assertion to test that lastIndex
> does not go above the Max string length?

1. For path
 js::RegExpTesterRaw test is done in function ExecuteRegExp():

>>    /* Handled by caller */
>>    MOZ_ASSERT(lastIndex >= 0 && size_t(lastIndex) <= input->length());
>>
>>    /* Steps 4-10 performed by the caller. */

2. For path js::RegExpMatcherRaw test is done in function RegExpMatcherImpl()->ExecuteRegExp()

So i guess we are safe to avoid any further checks.
Seems to me like lastIndex should be uint32_t.  I'm fairly sure we pass int32_t as uint32_t in some JIT entry points, and this seems like another place where that should be done.  That would also make the assertion reasonable.
(Assignee)

Comment 5

2 years ago
Created attachment 8715768 [details] [diff] [review]
as index cannot be negative make change it's storage from int32_t to uint32_t,
Attachment #8715768 - Flags: review?(jwalden+bmo)
Comment on attachment 8715768 [details] [diff] [review]
as index cannot be negative make change it's storage from int32_t to uint32_t,

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

::: js/src/builtin/RegExp.cpp
@@ +932,4 @@
>                       MatchPairs* maybeMatches, MutableHandleValue output)
> +{
> +    MOZ_ASSERT(lastIndex <= INT32_MAX);
> +    

Kill the trailing whitespace here.  (Not sure how any line but the |uint32_t lastIndex| line here got changed, to be honest.)

::: js/src/builtin/RegExp.h
@@ +55,5 @@
>  RegExpTester(JSContext* cx, unsigned argc, Value* vp);
>  
>  extern bool
>  RegExpTesterRaw(JSContext* cx, HandleObject regexp, HandleString input,
> +                uint32_t lastIndex, bool sticky, uint32_t* endIndex);

|endIndex| here shouldn't be changed to uint32_t.  RegExp testing returns -1 in the case where no match was found, so it makes sense for this to be signed.

::: js/src/jit/CodeGenerator.cpp
@@ +1915,5 @@
>      }
>  };
>  
>  typedef bool (*RegExpTesterRawFn)(JSContext* cx, HandleObject regexp, HandleString input,
> +                                  uint32_t lastIndex, bool sticky, uint32_t* result);

And then we should continue to have |int32_t* result| here.

::: js/src/jit/Recover.cpp
@@ +1040,5 @@
>      RootedString string(cx, iter.read().toString());
>      RootedObject regexp(cx, &iter.read().toObject());
>      int32_t lastIndex = iter.read().toInt32();
>      bool sticky = iter.read().toBoolean();
> +    uint32_t endIndex;

And |int32_t| again.
Attachment #8715768 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Updated

2 years ago
Attachment #8707389 - Attachment description: MozReview Request: Bug 1239269 - removed uselesss MOZ_ASSERT on lastIndex , r?jorendorff@mozilla.com → MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo
Attachment #8707389 - Flags: review?(jorendorff) → review?(jwalden+bmo)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8707389 [details]
MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30673/diff/1-2/
Comment on attachment 8707389 [details]
MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo

https://reviewboard.mozilla.org/r/30673/#review31155
Attachment #8707389 - Flags: review?(jwalden+bmo) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c7a71cce02

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97c7a71cce02
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.