Closed Bug 1239269 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1347680, CID 1347681)

Attachments

(2 files)

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.
Maybe it would make sense to change this assertion to test that lastIndex does not go above the Max string length?
(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.
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-
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)
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+
https://hg.mozilla.org/mozilla-central/rev/97c7a71cce02
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.