Last Comment Bug 464998 - integer overflow in nsEscape, still
: integer overflow in nsEscape, still
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.9.1, verified1.8.1.19, verified1.9.0.5
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla1.9.1
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on:
Blocks: 455987
  Show dependency treegraph
 
Reported: 2008-11-14 15:22 PST by Daniel Veditz [:dveditz]
Modified: 2009-04-20 10:55 PDT (History)
17 users (show)
mbeltzner: blocking1.9.1+
dveditz: blocking1.9.0.5+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x?
ted: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Get the sign right (918 bytes, patch)
2008-11-14 15:29 PST, Daniel Veditz [:dveditz]
benjamin: review+
benjamin: superreview+
mbeltzner: approval1.9.1b2+
samuel.sidler+old: approval1.8.1.19+
samuel.sidler+old: approval1.9.0.5+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2008-11-14 15:22:23 PST
+++ This bug was initially created as a clone of Bug #455987 +++

+++ This bug was initially created as a clone of Bug #445117 +++

bug 455987 tried to fix this, but I added when I should have subtracted the size of the terminator in nsEscapeHTML2.
Comment 1 Daniel Veditz [:dveditz] 2008-11-14 15:29:53 PST
Created attachment 348268 [details] [diff] [review]
Get the sign right
Comment 2 Doug Turner (:dougt) 2008-11-14 16:15:18 PST
The patch that wrote up looks right (based on the simple tests cases we ran it through), but it feels like a lot of work to ensure something bad doesn't happen a developer does something bad with the APIs.  

I think we should just redefine these APIs to fail of you pass more than x-Mb of data or some other huge but possibly reasonable number.  If you pass 4gb of data to nsEscapeHTML2, you are asking for trouble.  :-/
Comment 3 Doug Turner (:dougt) 2008-11-14 16:15:47 PST
s/that/that Dan
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2008-11-14 18:49:33 PST
Seems like it would be good to have xpcom/tests/TestEscape.cpp to test for these problems, no?  Then you know what's actually been tried and what hasn't been tried in the tests, and perhaps this error might have been caught.
Comment 5 Doug Turner (:dougt) 2008-11-14 19:02:51 PST
+1 great idea.
Comment 6 georgi - hopefully not receiving bugspam 2008-11-15 00:34:58 PST
> If you pass 4gb of data to nsEscapeHTML2, you are asking for trouble.  :-/

this is correct though known.
check:
Bug 367721 limiting virtual memory (mainly 64bit)
Bug 445295 misuse of int32 and int64 in API on 64 bit platforms
Comment 7 georgi - hopefully not receiving bugspam 2008-11-15 00:53:30 PST
offtopic:

does someone know how the ibm bug |ptr[negativeidx] = stuff;| was discovered - manual human inspection, static analyzer or else?

if it was static analyzer (which they may wish to promote for cash reasons) they may have more like this and probably *hydra checks for [negativeidx] may be considered.
Comment 8 Samuel Sidler (old account; do not CC) 2008-11-17 12:47:24 PST
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

Approved for 1.9.0.5 and 1.8.1.19. a=ss for release-drivers
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-17 21:51:04 PST
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a191b2=beltzner
Comment 10 Daniel Veditz [:dveditz] 2008-11-18 12:46:54 PST
Fix checked into 1.8 and 1.9.0 branches
Comment 11 Ted Mielczarek [:ted.mielczarek] 2008-11-19 09:18:28 PST
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/79948e2ddec5
Comment 12 Doug Turner (:dougt) 2008-11-19 09:20:27 PST
no tests?  qq
Comment 13 georgi - hopefully not receiving bugspam 2008-11-20 00:07:21 PST
>no tests?  qq

there is a testcase in Bug 445117 for basically the same, though it is a perl script generating folder. testcases for this bug will use significant amount of VM.
Comment 14 Al Billings [:abillings] 2008-11-25 17:44:11 PST
Should I trust your ability to change the sign, Dan? :-)
Comment 16 Al Billings [:abillings] 2008-11-25 17:52:59 PST
You take the fun out of teasing Mr. Veditz but, yes, it is clearly fixed.
Comment 17 Alexander Sack 2008-12-16 01:10:41 PST
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a=asac for 1.8.0
Comment 18 Aakash Desai [:aakashd] 2009-04-20 10:55:47 PDT
Hey Al, can you verify this bug across trunk and 1.9.1? I don't have access to bug 445117 due to security clearances :(

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