Closed Bug 408630 Opened 13 years ago Closed 9 years ago

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


(Core :: Layout, defect, trivial)

Not set





(Reporter: masa141421356, Assigned: atulagrwl)



(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv: Gecko/20071127 Firefox/
Build Identifier: 


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.
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.
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.
If length = MAX_INT, Truncate() ssems to be called by argeument MIN_INT.
(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.
I think this bug can be detected by Compiler's warning configuration.
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
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
Ever confirmed: true
Attached patch Patch v1Splinter Review
Attachment #571880 - Flags: review?(roc)
Keywords: checkin-needed
Keywords: helpwanted
Attached patch problem fixed (obsolete) — Splinter Review
Attachment #572273 - Flags: review?(roc)
(In reply to Ed Morley [:edmorley] from comment #14)

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.
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: abhishekkumarsingh.cse → atulagrwl
Closed: 9 years ago
Resolution: --- → FIXED
It's not identical:
Resolution: FIXED → ---
My mistake. I should look more closely :).
Assignee: atulagrwl → abhishekkumarsingh.cse
Attachment #572273 - Attachment is obsolete: false
Attachment #572273 - Flags: review?(roc)
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:
Assignee: abhishekkumarsingh.cse → atulagrwl
Closed: 9 years ago9 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.