664920 - Audit yarr for int/int64 problems

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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?]

Comment 2

[Daniel Veditz [:dveditz]]

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?

Comment 3

[Daniel Veditz [:dveditz]]

Webkit opened https://bugs.webkit.org/show_bug.cgi?id=63297 on the m_checked issue. We should still look for more.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[David Mandelin [:dmandelin]]

(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

[Asa Dotzler [:asa]]

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.

Comment 7

[Johnny Stenback (:jst)]

Any updates on the Apple side here? If not, what can we do to move forward here?

Comment 8

[Daniel Veditz [:dveditz]]

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?

Comment 9

[David Mandelin [:dmandelin]]

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.

Comment 10

[Daniel Veditz [:dveditz]]

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.