"part 5" in bug 966166 removed the fixed version of CJKIdeographicToText which lived in layout/generic/nsBulletFrame.cpp and instead landed the old version of CJKIdeographicToText without the fix for bug 964078 in layout/style/CounterStyleManager.cpp https://bugzilla.mozilla.org/attachment.cgi?id=8438903&action=diff Instead there is now a NS_ASSERTION(aOrdinal >= 0, "Only accept non-negative ordinal"); which fails for the testcase in bug 964078.
Xidorn, did you revert the fix from bug 964078 intentionally? If so, what is it that is supposed to guarantee that aOrdinal is non-negative exactly?
Severity: normal → critical
OS: Windows 7 → All
Hardware: x86 → All
I cannot see content of bug 964078, could you cc that to me?
Oh, sorry about that, I have CC:ed you know.
FYI, it's only "Android 4.0 API11+ debug" that failed, so maybe something is odd with that platform? https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e3140297b2
OK, yes, it is supposed to guarantee that aOrdinal is non-negative, because after bug 966166, the negative sign is handled in another path. And that assert could happen on Android platform because bug 989718 that the ARM compiler invalidates the CheckedInt used in CounterStyle::GetCounterText.  This problem also causes some known reftest failures in layout/reftests/counters.   https://dxr.mozilla.org/mozilla-central/source/layout/style/CounterStyleManager.cpp#1866  https://dxr.mozilla.org/mozilla-central/source/layout/reftests/counters/reftest.list#63
OK, so I guess that means bug 964078 still occurs on that platform. So we should probably try to fix bug 989718 with higher priority, or add a wallpaper in CJKIdeographicToText using a uint32_t like I did in bug 964078, would that work?
Depends on: 989718
The best way would be fixing bug 989718. Instead of wrapping aOrdinal, I'd prefer changing int32_t cur = aOrdinal % 10; to something like: auto cur = static_cast<MakeUnsigned<CounterValue>::Type>(aOrdinal) % 10;
This won't fix the reftest failures and assertions. But I guess it's probably better to leave them as they are now, because we don't actually fix the problem before we fix bug 989718.
Attachment #8567592 - Flags: review?(mats)
Attachment #8567592 - Flags: review?(mats) → review+
Comment on attachment 8567592 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]: bug 966166 [User impact if declined]: may be exposed to buffer overflow issue on ARM platforms where we use the buggy compiler [Describe test coverage new/current, TreeHerder]: no test [Risks and why]: no risk [String/UUID change made/needed]: n/a
In bug 964078 we concluded that it isn't a very serious issue though. See analysis in bug 964078 comment 12.
I'm not sure, because the worst case is using gDataJapaneseInformal. What is gDataJapaneseInformal.digits[-9] is unknown, because it is the first table in that file. But it might still be fine, because it is highly possible that it is still some static data.
Comment on attachment 8567592 [details] [diff] [review] patch Too late for release, we build rc and for now at least, we don't have a reason to rebuild a second rc.
Attachment #8567592 - Flags: approval-mozilla-release? → approval-mozilla-release-
I'm marking both 35 (EOL tomorrow) and 36, which is scheduled to release tomorrow, as wontfix. We can look at taking this fix in 37.
Comment on attachment 8567592 [details] [diff] [review] patch Clearing Aurora approval as Aurora is now 38, which is already fixed.
mats, xidorn - From comment 11 and comment 12 I'm not sure you two are on the same page wrt this fix. Is there a good reason not to uplift this fix to 37?
The out-of-bounds read is limited to 9 bytes. Even if gDataJapaneseInformal is first in that section, so you might be able to read a few bytes off the end of the previous memory page, it still sec-low IMO. It might cause crashes if that address isn't mapped. It only affects platforms where we use a buggy compiler (bug 989718). It doesn't seem worth uploading to Beta to me.
OS: All → Android
Hardware: All → ARM
I have no idea. If mats says it's not important then it's not important, although I think it's trivial to uplift.
Comment on attachment 8567592 [details] [diff] [review] patch OK. If this isn't required, let's let it ride the 38 train. Thank you both for you input on this and Xidorn for the fix! Beta-
Attachment #8567592 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.