xslt number formatting function crashes on big numbers [@ txRomanCounter::appendNumber(int, nsAString_internal&) ]

RESOLVED FIXED

Status

()

Core
XSLT
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Sergey Glazunov, Assigned: bz)

Tracking

(4 keywords)

1.9.2 Branch
x86
Windows 7
crash, testcase, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .7+, status1.9.2 .7-fixed, blocking1.9.1 .11+, status1.9.1 .11-fixed)

Details

(crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.89 Safari/532.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6

Mozilla crashes while transforms a large number (like 1000000000000000) into the Roman numeral system using XSLT (<xsl:number value="1000000000000000" format="i"/>).

Reproducible: Always
(Reporter)

Comment 1

8 years ago
Created attachment 426745 [details]
malformed xslt document
(Reporter)

Comment 2

8 years ago
Created attachment 426746 [details]
xml file to open
I don't crash using  Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6. I tried using a trunk build and it seemed when I tried to shut down that Minefield closed. Do you have a crash report from about:crashes (in the URL bar)?
(Reporter)

Comment 4

8 years ago
http://crash-stats.mozilla.com/report/index/8519a2ce-2535-46f5-80b3-31a892100213
(Reporter)

Comment 5

8 years ago
i found out where the bug in the code is.
there is the check in the txRomanCounter::appendNumber method:
if (aNumber >= 4000) {
but aNumber is considered as signed int, so it passes the check if it equals at least 2147483648.
0  	xul.dll  	txRomanCounter::appendNumber  	 content/xslt/src/xslt/txXSLTNumberCounters.cpp:239
1 	xul.dll 	txXSLTNumber::createNumber 	content/xslt/src/xslt/txXSLTNumber.cpp:94
2 	xul.dll 	txNumber::execute 	content/xslt/src/xslt/txInstructions.cpp:597
3 	xul.dll 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
4 	xul.dll 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:683
Status: UNCONFIRMED → NEW
Component: General → XSLT
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
QA Contact: general → xslt
Summary: xslt number formatting function crashes on big numbers → xslt number formatting function crashes on big numbers [@ txRomanCounter::appendNumber(int, nsAString_internal&) ]
Version: unspecified → 1.9.2 Branch
(Reporter)

Comment 7

8 years ago
Created attachment 426838 [details]
one file javascript version
Attachment #426745 - Attachment is obsolete: true
Attachment #426746 - Attachment is obsolete: true
Created attachment 427251 [details] [diff] [review]
Proposed fix

Serg, thank you for the testcase and analysis!
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #427251 - Flags: review?(jonas)
Comment on attachment 427251 [details] [diff] [review]
Proposed fix

Sweet, thanks guys!
Attachment #427251 - Flags: review?(jonas) → review+

Updated

8 years ago
Keywords: testcase
Pushed as http://hg.mozilla.org/mozilla-central/rev/4c8923d18e2e

I guess we should try to get this in on the branch too....
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Comment on attachment 427251 [details] [diff] [review]
Proposed fix

Requesting approval for this "don't try to read memory we haven't mapped" fix.
Attachment #427251 - Flags: approval1.9.2.2?
Comment on attachment 427251 [details] [diff] [review]
Proposed fix

Uhm, isn't that the very definition of a security bug?
Attachment #427251 - Flags: approval1.9.2.2? → approval1.9.2.3?
> Uhm, isn't that the very definition of a security bug?

Well, it can be, yes.
blocking1.9.2: --- → ?
Attachment #427251 - Flags: approval1.9.2.4? → approval1.9.2.5?
Is the 1.9.1 branch affected as well?
blocking1.9.1: --- → ?
blocking1.9.2: ? → .5+
status1.9.1: --- → ?
status1.9.2: --- → wanted
Attachment #427251 - Flags: approval1.9.2.5? → approval1.9.2.5+
Comment on attachment 427251 [details] [diff] [review]
Proposed fix

Approved for 1.9.2.5, a=dveditz for release-drivers

Updated

7 years ago
blocking1.9.1: ? → .11+
blocking1.9.2: .5+ → .6+
status1.9.1: ? → wanted
> Is the 1.9.1 branch affected as well?

Yes.
Attachment #427251 - Flags: approval1.9.1.11?
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/db34c710c17e

Here's hoping I'm setting the status flag right... (i.e. NOT matching the approval flag).
status1.9.2: wanted → .6-fixed
Comment on attachment 427251 [details] [diff] [review]
Proposed fix

You did that right, thanks. We're a little off-kilter due to the decision to split Fennec 1.1 off mid-release.

Approved for 1.9.1.11, a=dveditz
Attachment #427251 - Flags: approval1.9.2.6+
Attachment #427251 - Flags: approval1.9.2.5+
Attachment #427251 - Flags: approval1.9.1.11?
Attachment #427251 - Flags: approval1.9.1.11+
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3d53e20b39a5
status1.9.1: wanted → .11-fixed
Verified for 1.9.1.11 using testcase in comment 7 (which crashes 1.9.1.10): Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.11) Gecko/20100701 Firefox/3.5.11 (.NET CLR 3.5.30729).

Verified for 1.9.2.7 the same way with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729).
Keywords: verified1.9.1, verified1.9.2
Crash Signature: [@ txRomanCounter::appendNumber(int, nsAString_internal&) ]
You need to log in before you can comment on or make changes to this bug.