Bug 966166 reverted the fix for bug 964078

RESOLVED FIXED in Firefox 38

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mats, Assigned: xidorn)

Tracking

(5 keywords)

Trunk
mozilla38
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-master fixed)

Details

(Whiteboard: [adv-main38-])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
"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.
(Reporter)

Comment 1

4 years ago
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
Flags: needinfo?(quanxunzhen)
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 2

4 years ago
I cannot see content of bug 964078, could you cc that to me?
(Assignee)

Updated

4 years ago
Flags: needinfo?(mats)
(Reporter)

Comment 3

4 years ago
Oh, sorry about that, I have CC:ed you know.
Flags: needinfo?(mats)
(Reporter)

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
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
Flags: needinfo?(quanxunzhen)
(Reporter)

Comment 6

4 years ago
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
(Assignee)

Comment 7

4 years ago
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;
(Assignee)

Comment 8

4 years ago
Posted patch patchSplinter Review
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)
(Reporter)

Updated

4 years ago
Attachment #8567592 - Flags: review?(mats) → review+
(Assignee)

Comment 10

4 years ago
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
Attachment #8567592 - Flags: approval-mozilla-release?
Attachment #8567592 - Flags: approval-mozilla-beta?
Attachment #8567592 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 11

4 years ago
In bug 964078 we concluded that it isn't a very serious issue though.
See analysis in bug 964078 comment 12.
(Assignee)

Comment 12

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/9e46e93f81dd
Assignee: nobody → quanxunzhen
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8567592 - Flags: approval-mozilla-aurora?
Group: layout-core-security → core-security
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?
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(mats)
(Reporter)

Comment 18

4 years ago
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.
Flags: needinfo?(mats)
OS: All → Android
Hardware: All → ARM
(Assignee)

Comment 19

4 years ago
I have no idea. If mats says it's not important then it's not important, although I think it's trivial to uplift.
Flags: needinfo?(quanxunzhen)
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-
Whiteboard: [adv-main38-]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.