Closed Bug 1226549 Opened 4 years ago Closed 4 years ago

[Static Analysis][Called C++ object pointer is null] ExecuteRegExp from js/src/builtin/RegExp.cpp Called C++ object pointer is null

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: clang-analyzer)

Attachments

(1 file, 3 obsolete files)

The Static Analysis tool Scan-Build added a null pointer error in  ExecuteRegExp from js/src/builtin/RegExp.cpp - line 817.
Attached patch Bug 1226549.diff (obsolete) — Splinter Review
Assignee: nobody → bogdan.postelnicu
Thanks for the report! Did you forget to qref maybe? The patch appears to be empty.
Flags: needinfo?(bogdan.postelnicu)
Attached patch Bug 1226549.diff (obsolete) — Splinter Review
You are right something strange happened, this is the right version.
Flags: needinfo?(bogdan.postelnicu)
Attachment #8690025 - Attachment is obsolete: true
cc-ing h4writer as he knows the RegExp code the best.
The code there indeed use some strange / hard to follow control flow. But after looking I still think it is correct. Off course it cannot hurt to add an MOZ_ASSERT
Comment on attachment 8690042 [details] [diff] [review]
Bug 1226549.diff

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

Let's see what try says about this assertion. I am not sure that this is always true by just reading the code, but I don't know it very well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a07b091db4

::: js/src/builtin/RegExp.cpp
@@ +813,5 @@
>          if (!SetLastIndex(cx, reobj, 0))
>              return RegExpRunStatus_Error;
>      } else if (reobj->needUpdateLastIndex()) {
>          /* Steps 18.a-b. */
> +		MOZ_ASSERT(matches);

nit: please indent the same as the 'if' below

@@ +818,4 @@
>          if (!SetLastIndex(cx, reobj, (*matches)[0].limit))
>              return RegExpRunStatus_Error;
>      }
> +	

nit: trailing spaces here
Attached patch Bug 1226549.diff (obsolete) — Splinter Review
removed useless spaces
Attached patch Bug 1226549.diffSplinter Review
Added to the assert the check to see if pairCount_ > 0 since this is true we know for sure that pairs_ will not point to null toughs avoiding bad access in function: operator [], overloaded by class MatchPairs
Comment on attachment 8691951 [details] [diff] [review]
Bug 1226549.diff

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

That looks sane, and try confirms. Hannes, what do you think?
Attachment #8691951 - Flags: review?(hv1989)
Attachment #8691904 - Attachment is obsolete: true
Attachment #8690042 - Attachment is obsolete: true
Comment on attachment 8691951 [details] [diff] [review]
Bug 1226549.diff

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

looks fine to me.
Attachment #8691951 - Flags: review?(hv1989) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69b0a3677dc1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.