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...
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
-- trivial (vote)
: mozilla10
Assigned To: Atul Aggarwal
: Jet Villegas (:jet)
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image Masahiro YAMADA 2007-12-16 19:38:32 PST
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.
Comment 1 User image Masahiro YAMADA 2007-12-16 20:05:33 PST
According to

1.8Branch has same bug.
Comment 2 User image 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 User image Masahiro YAMADA 2007-12-16 20:31:33 PST
If length = MAX_INT, Truncate() ssems to be called by argeument MIN_INT.
Comment 4 User image 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 User image Masahiro YAMADA 2007-12-17 15:42:09 PST
I think this bug can be detected by Compiler's warning configuration.
Comment 6 User image 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 User image 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 User image 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 User image 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 User image Tanner M. Young [:tmyoung] 2009-07-23 17:27:54 PDT
Or rather issue, not crash in this case...
Comment 11 User image 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 User image Atul Aggarwal 2011-11-03 21:32:40 PDT
Created attachment 571880 [details] [diff] [review]
Patch v1
Comment 13 User image Ed Morley [:emorley] 2011-11-05 03:45:17 PDT
Comment 15 User image Abhishek Singh 2011-11-06 01:28:11 PDT
Created attachment 572273 [details] [diff] [review]
problem fixed
Comment 16 User image Ed Morley [:emorley] 2011-11-06 05:30:14 PST
(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 17 User image 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 User image Atul Aggarwal 2011-11-06 05:47:41 PST
My mistake. I should look more closely :).
Comment 20 User image 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:
Comment 21 User image David Baron :dbaron: ⌚️UTC-8 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 User image 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.