Bugzilla@Mozilla – Bug 464998
integer overflow in nsEscape, still
Last modified: 2009-04-20 10:55:47 PDT
Summon comment box
+++ 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.
Created attachment 348268 [details] [review] Get the sign right
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. :-/
s/that/that Dan
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.
+1 great idea.
> 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
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 on attachment 348268 [details] [review] Get the sign right Approved for 1.9.0.5 and 1.8.1.19. a=ss for release-drivers
Comment on attachment 348268 [details] [review] Get the sign right a191b2=beltzner
Fix checked into 1.8 and 1.9.0 branches
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/79948e2ddec5
no tests? qq
>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.
Should I trust your ability to change the sign, Dan? :-)
You can also check Bonsai. 1.9.0: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-11-18+12%3A45&maxdate=2008-11-18+12%3A45&cvsroot=%2Fcvsroot 1.8.1: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-11-18+12%3A45&maxdate=2008-11-18+12%3A45&cvsroot=%2Fcvsroot
You take the fun out of teasing Mr. Veditz but, yes, it is clearly fixed.
Comment on attachment 348268 [details] [review] Get the sign right a=asac for 1.8.0
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 :(