464998 - integer overflow in nsEscape, still

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Veditz [:dveditz]]

+++ 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]]

Attachment: Get the sign right

Comment 2

[Doug Turner (:dougt)]

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)]

s/that/that Dan

Comment 4

[Jeff Walden [:Waldo]]

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)]

+1 great idea.

Comment 6

[georgi - hopefully not receiving bugspam]

> 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]

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)]

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]]

Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a191b2=beltzner

Comment 10

[Daniel Veditz [:dveditz]]

Fix checked into 1.8 and 1.9.0 branches

Comment 11

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/79948e2ddec5

Comment 12

[Doug Turner (:dougt)]

no tests?  qq

Comment 13

[georgi - hopefully not receiving bugspam]

>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 - ex-MoCo]]

Should I trust your ability to change the sign, Dan? :-)

Comment 15

[Samuel Sidler (old account; do not CC)]

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

Comment 16

[Al Billings [:abillings - ex-MoCo]]

You take the fun out of teasing Mr. Veditz but, yes, it is clearly fixed.

Comment 17

[Alexander Sack]

Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a=asac for 1.8.0

Comment 18

[Aakash Desai [:aakashd]]

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 :(