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

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {clang-analyzer})

Trunk
mozilla45
clang-analyzer
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
The Static Analysis tool Scan-Build added a null pointer error in  ExecuteRegExp from js/src/builtin/RegExp.cpp - line 817.
(Assignee)

Updated

3 years ago
Assignee: nobody → bogdan.postelnicu
Thanks for the report! Did you forget to qref maybe? The patch appears to be empty.
Flags: needinfo?(bogdan.postelnicu)
(Assignee)

Comment 3

3 years ago
Created attachment 8690042 [details] [diff] [review]
Bug 1226549.diff

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
Keywords: clang-analyzer
Blocks: 712350
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
(Assignee)

Comment 7

2 years ago
Created attachment 8691904 [details] [diff] [review]
Bug 1226549.diff

removed useless spaces
(Assignee)

Comment 8

2 years ago
Created attachment 8691951 [details] [diff] [review]
Bug 1226549.diff

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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

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