Closed
Bug 664920
Opened 14 years ago
Closed 13 years ago
Audit yarr for int/int64 problems
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox5 | - | wontfix |
firefox6 | - | wontfix |
firefox7 | - | wontfix |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: dveditz, Assigned: dmandelin)
References
Details
(Whiteboard: [sg:audit])
Yarr likely has several recurrences of the pattern that caused us problems in bug 653672 and needs to be audited for it before we land that patch.
From Bug #653672 comment 8:
One thing of note is that int64 won't be wider than int on ILP64 systems (like SPARC), but there are also a number of other cases where YARR-JIT subtracts unchecked-value unsigned offsets from each other and sticks them into an int. Most of the places where you see the pattern |int result = thing - other;| in this file is a potential candidate for overflow. Maybe instead of committing this we should perform a full audit? Things like setupAlternative/DisjunctionOffsets and backtrack have similar errors. IMO we should try to go through and construct a test case to demonstrate each one to prevent further regressions.
Reporter | ||
Comment 1•14 years ago
|
||
If unsigned m_minimumSize can get big enough to cause the problem in bug 653672 I also see a number of worrying
m_checked += alternative->m_minimumSize;
where m_checked is an int.
Although written as an [sg:audit] bug I think we've seen enough instances of the pattern to call it [sg:critical?]
Keywords: testcase-wanted
Whiteboard: [sg:critical?]
Reporter | ||
Comment 2•14 years ago
|
||
bug 653672 got a more extensive patch, but didn't change anything around the question I had in comment 1. Is there still more to do here, and if so who should do it?
Reporter | ||
Comment 3•14 years ago
|
||
Webkit opened https://bugs.webkit.org/show_bug.cgi?id=63297 on the m_checked issue. We should still look for more.
See Also: → https://bugs.webkit.org/show_bug.cgi?id=63297
Updated•14 years ago
|
status-firefox8:
--- → affected
tracking-firefox8:
--- → +
Reporter | ||
Comment 4•14 years ago
|
||
dmandelin: did webkit patch this in their code? if so we also need to land a fix in Fx6, if not Fx7 might be OK.
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4)
> dmandelin: did webkit patch this in their code? if so we also need to land a
> fix in Fx6, if not Fx7 might be OK.
They have a bug open, I have seen any activity lately.
Comment 6•14 years ago
|
||
Is there anything that needs to be done here for Firefox 6? If so, who is the right person to be doing that? We've only got a few weeks left.
Updated•14 years ago
|
Comment 7•14 years ago
|
||
Any updates on the Apple side here? If not, what can we do to move forward here?
Reporter | ||
Comment 8•13 years ago
|
||
dmandelin: jst says you and cdleary spent some time looking for the problem patterns we saw in the other bug. Want to add a status report here and call it done?
Updated•13 years ago
|
Assignee | ||
Comment 9•13 years ago
|
||
cdleary and I stared at it for a little while and didn't find anything too interesting. I looked at the other uses of m_checked and m_alternativeSize and didn't see anything particularly interesting. It would really take a lot more effort to really know if there's anything left, though.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INCOMPLETE
Reporter | ||
Comment 10•13 years ago
|
||
I also read through all of Yarr. It's been heavily revised since the original bug and I couldn't find any occurrences of the problematic patterns.
Resolution: INCOMPLETE → FIXED
Whiteboard: [sg:critical?] → [sg:audit]
status-firefox8:
affected → ---
tracking-firefox8:
+ → ---
Reporter | ||
Updated•10 years ago
|
Group: core-security
Updated•9 years ago
|
Keywords: testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•