inserting ellipsis to VERY long string (Length>MAX_INT) works wrong by integer overflow.

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
--
trivial
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Masahiro YAMADA, Assigned: Atul Aggarwal)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: 

See 
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#1376

1376           int length = aText.Length();
1377           int i;
1378           for (i = 0; i < length; ++i) {

It uses "int" , But Length of text can be larger then MAX_INT if int is 16bit.
Line 1395-1397,1417-1419 has same problem.

Reproducible: Always

Steps to Reproduce:
See source.
Actual Results:  
"int" is used.

Expected Results:  
They should be 32bit int like PRInt32.

Line  1417-1419 is very critical because it may causes buffer overflow.
It is resaon to set security flag.

I found a testcase of this .
It is bug 407821.
(Reporter)

Updated

10 years ago
Summary: inserting ellipsis to VERY long (Length>MAX_INT) works long because of integer overflow. → inserting ellipsis to VERY long string (Length>MAX_INT) works wrong by integer overflow.
(Reporter)

Comment 1

10 years ago
According to http://lxr.mozilla.org/mozilla1.8.0/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2819

1.8Branch has same bug.
(Reporter)

Comment 2

10 years ago
I don't know this bug can causes buffer overflow/undeflow or cannot(seems to be cannot)

If this bug can not causes buffer overflow/undeflow,
there is no reason to keep security-sensitive.
(Reporter)

Comment 3

10 years ago
If length = MAX_INT, Truncate() ssems to be called by argeument MIN_INT.
(Reporter)

Comment 4

10 years ago
(In reply to comment #3)
> If length = MAX_INT, Truncate() ssems to be called by argeument MIN_INT.
> 
It is almostly not correct. because display width is limited.
(Reporter)

Comment 5

10 years ago
I think this bug can be detected by Compiler's warning configuration.
(Reporter)

Comment 6

10 years ago
If Length of String is 32769, 
"aText.Right(copy, length-1-i)" in line:1406 seems to be called as aText.Right(copy,-65534).

Is it safe?
On which of our supported platforms is "int" ever 16-bit? Still, wouldn't hurt to make these explicitly PRUint32
Group: security
Component: General → Layout
QA Contact: general → layout
(Reporter)

Comment 8

9 years ago
If "int" is 32bit on Trunk Windows build, bug about VERY LONG tag at Library(Places Organizer) exists in other code (See attachment 294668 [details], tag should be cropped but it is not cropped).
And, according to MSDN Library, "int" of VC++2005 seems to be 32bit.
Is it correct?

Is this crash still reproducible in Firefox 3.5?  If it isn't, we should resolve it as RESOLVED WORKSFORME.
Or rather issue, not crash in this case...
I'm pretty sure we will never support platforms with a 16-bit 'int'.
As Daniel said, replacing it with PRUint32 doesn't hurt though.
Severity: critical → trivial
Keywords: helpwanted
Whiteboard: [good first bug]
Assignee: nobody → abhishekkumarsingh.cse
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 12

6 years ago
Created attachment 571880 [details] [diff] [review]
Patch v1
Attachment #571880 - Flags: review?(roc)
Attachment #571880 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=7cd51cfc473a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e800d88af77
Keywords: checkin-needed
Target Milestone: --- → mozilla10
Keywords: helpwanted

Comment 15

6 years ago
Created attachment 572273 [details] [diff] [review]
problem fixed
Attachment #572273 - Flags: review?(roc)
(In reply to Ed Morley [:edmorley] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6e800d88af77

https://hg.mozilla.org/mozilla-central/rev/6e800d88af77

Although confused given there's a new patch in comment 15?
Why the new patch?
+ it will need to be rebased given the first one has landed.

Leaving open until that sorted out.
(Assignee)

Comment 17

6 years ago
Comment on attachment 572273 [details] [diff] [review]
problem fixed

New patch is exact duplicate to previous patch.
Hence marking new patch obsolete and closing this bug.
Attachment #572273 - Attachment is obsolete: true
Attachment #572273 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Assignee: abhishekkumarsingh.cse → atulagrwl
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
It's not identical:
https://bugzilla.mozilla.org/attachment.cgi?oldid=571880&action=interdiff&newid=572273&headers=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

6 years ago
My mistake. I should look more closely :).
Assignee: atulagrwl → abhishekkumarsingh.cse
(Assignee)

Updated

6 years ago
Attachment #572273 - Attachment is obsolete: false
Attachment #572273 - Flags: review?(roc)

Updated

6 years ago
Attachment #572273 - Attachment is obsolete: true
Attachment #572273 - Flags: review?(roc)
There's no need for a new patch here.  The first one fixed it and has landed
on mozilla-inbound and has now been merged to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/6e800d88af77
Assignee: abhishekkumarsingh.cse → atulagrwl
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
The first patch, which landed, used PRUint32 (unsigned) in the "Crop right" case, but PRInt32 (signed) in the "Crop left" and "Crop center" cases.  The second patch uses PRUint32 in all three cases.  If the intent was to use PRUint32, then more changes are needed.
The intent was to use types that are guaranteed to be 32-bit.
Changing the signed types to unsigned would introduce bugs unless
the code is re-written.  Please, let's not waste time on that.
You need to log in before you can comment on or make changes to this bug.