Last Comment Bug 408630 - inserting ellipsis to VERY long string (Length>MAX_INT) works wrong by integer overflow.
: inserting ellipsis to VERY long string (Length>MAX_INT) works wrong by intege...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: mozilla10
Assigned To: Atul Aggarwal
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-16 19:38 PST by Masahiro YAMADA
Modified: 2011-11-06 11:19 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.47 KB, patch)
2011-11-03 21:32 PDT, Atul Aggarwal
roc: review+
Details | Diff | Review
problem fixed (2.26 KB, patch)
2011-11-06 01:28 PDT, Abhishek Singh
no flags Details | Diff | Review

Description Masahiro YAMADA 2007-12-16 19:38:32 PST
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.
Comment 1 Masahiro YAMADA 2007-12-16 20:05:33 PST
According to http://lxr.mozilla.org/mozilla1.8.0/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2819

1.8Branch has same bug.
Comment 2 Masahiro YAMADA 2007-12-16 20:12:59 PST
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.
Comment 3 Masahiro YAMADA 2007-12-16 20:31:33 PST
If length = MAX_INT, Truncate() ssems to be called by argeument MIN_INT.
Comment 4 Masahiro YAMADA 2007-12-16 21:18:04 PST
(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.
Comment 5 Masahiro YAMADA 2007-12-17 15:42:09 PST
I think this bug can be detected by Compiler's warning configuration.
Comment 6 Masahiro YAMADA 2007-12-18 01:21:00 PST
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?
Comment 7 Daniel Veditz [:dveditz] 2008-01-11 00:30:41 PST
On which of our supported platforms is "int" ever 16-bit? Still, wouldn't hurt to make these explicitly PRUint32
Comment 8 Masahiro YAMADA 2008-01-11 01:48:08 PST
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?

Comment 9 Tanner M. Young [:tmyoung] 2009-07-23 17:26:32 PDT
Is this crash still reproducible in Firefox 3.5?  If it isn't, we should resolve it as RESOLVED WORKSFORME.
Comment 10 Tanner M. Young [:tmyoung] 2009-07-23 17:27:54 PDT
Or rather issue, not crash in this case...
Comment 11 Mats Palmgren (:mats) 2011-08-13 21:17:10 PDT
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.
Comment 12 Atul Aggarwal 2011-11-03 21:32:40 PDT
Created attachment 571880 [details] [diff] [review]
Patch v1
Comment 13 Ed Morley [:emorley] 2011-11-05 03:45:17 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7cd51cfc473a
Comment 15 Abhishek Singh 2011-11-06 01:28:11 PDT
Created attachment 572273 [details] [diff] [review]
problem fixed
Comment 16 Ed Morley [:emorley] 2011-11-06 05:30:14 PST
(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.
Comment 17 Atul Aggarwal 2011-11-06 05:36:19 PST
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.
Comment 19 Atul Aggarwal 2011-11-06 05:47:41 PST
My mistake. I should look more closely :).
Comment 20 Mats Palmgren (:mats) 2011-11-06 06:12:02 PST
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
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-11-06 10:38:07 PST
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.
Comment 22 Mats Palmgren (:mats) 2011-11-06 11:19:55 PST
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.

Note You need to log in before you can comment on or make changes to this bug.