1135426 - Bug 966166 reverted the fix for bug 964078

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

"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.

Comment 1

[Mats Palmgren (inactive)]

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?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I cannot see content of bug 964078, could you cc that to me?

Comment 3

[Mats Palmgren (inactive)]

Oh, sorry about that, I have CC:ed you know.

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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. [1] This problem also causes some known reftest failures in layout/reftests/counters. [2]

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/CounterStyleManager.cpp#1866
[2] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/counters/reftest.list#63

Comment 6

[Mats Palmgren (inactive)]

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?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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;

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e46e93f81dd

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 11

[Mats Palmgren (inactive)]

In bug 964078 we concluded that it isn't a very serious issue though.
See analysis in bug 964078 comment 12.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

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 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e46e93f81dd

Comment 14

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

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 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567592 [details] [diff] [review]
patch

Clearing Aurora approval as Aurora is now 38, which is already fixed.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 18

[Mats Palmgren (inactive)]

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.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

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 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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-