Closed
Bug 1226549
Opened 9 years ago
Closed 8 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)
Core
JavaScript Engine
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)
892 bytes,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
The Static Analysis tool Scan-Build added a null pointer error in ExecuteRegExp from js/src/builtin/RegExp.cpp - line 817.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bogdan.postelnicu
Comment 2•9 years ago
|
||
Thanks for the report! Did you forget to qref maybe? The patch appears to be empty.
Flags: needinfo?(bogdan.postelnicu)
Assignee | ||
Comment 3•9 years ago
|
||
You are right something strange happened, this is the right version.
Flags: needinfo?(bogdan.postelnicu)
Updated•9 years ago
|
Attachment #8690025 -
Attachment is obsolete: true
Comment 4•9 years ago
|
||
cc-ing h4writer as he knows the RegExp code the best.
Comment 5•9 years ago
|
||
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
Updated•9 years ago
|
Keywords: clang-analyzer
Updated•9 years ago
|
Blocks: clang-based-analysis
Comment 6•9 years ago
|
||
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•9 years ago
|
||
removed useless spaces
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
I've pushed the new patch to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=32153c767c31
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8691904 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8690042 -
Attachment is obsolete: true
Comment 11•9 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/69b0a3677dc1
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69b0a3677dc1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•