508074 - nsBinHexDecoder should use an nsCString for mName

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[timeless]

http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsBinHexDecoder.cpp?mark=206,210,212#184

steps:
1. load http://download.adobe.com/pub/adobe/golive/mac/6.x/GL6.0.1_Mac_Client_Updater.hqx

expected results:
unclear

actual results (on arm):
double free in destructor

184 nsresult nsBinHexDecoder::ProcessNextState(nsIRequest * aRequest, nsISupports * aContext)

204       // c & 63 returns the length of mName. So if we need the length, that's how
205       // you can figure it out...for now we are storing it in the first byte of mName
206       *(mName) = (c & 63);     

this can set a value of up to 63


let mCount = 62

209     case BINHEX_STATE_FNAME:
210       mName[mCount] = c;

here we stomp on values. mCount == 62

212       if (mCount++ > *(mName)) // the first byte of mName is the length...

if 62 > 63 ... no,
mCount = 63

210       mName[mCount] = c;

here we stomp on values. mCount == 63

if 63 > 63 ... no
mCount = 64

210       mName[mCount] = c;

here we stomp on values. mCount == 64
oops, our array is only 0..63

if 64 > 63 ... yes
mCount = 65

Comment 1

[timeless]

Attachment: patch

Comment 2

[timeless]

Attachment: mq header ....

Comment 3

[timeless]

Attachment: i need dinner

Comment 4

[timeless]

Attachment: using an nsCString

this patch requires the patch from bug 508229.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 392465 [details] [diff] [review]
using an nsCString

+      mName.SetLength(c & 63);
       break;

perhaps check that this succeeded:
  if (mName.Length() != (c & 63)) {
    // report error
  }

Comment 6

[timeless]

Attachment: with basic handling

so, this is the best i can do short of rewriting a large portion of that code, and biesi thinks it's the better choice for now.

i have a draft which goes toward that goal, but doesn't reach it. note that this code technically violates necko apis, but we're going to live with that (i believe the existing code does too, but in other ways).

Comment 7

[timeless]

Attachment: with both files....

Comment 8

[timeless]

http://hg.mozilla.org/mozilla-central/rev/f433451f8c40

Comment 9

[Daniel Veditz [:dveditz]]

testcase-wanted -- reduced if possible, but if nothing else capture the URL so we have a test in case the remote resource goes away.

Please request approval for the branches you want to land this on.

Comment 10

[Bob Clary [:bc] (inactive)]

jmaher, I can save the page but not test that it actually reproduces the problem without a machine. Can you take a look?

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 392720 [details] [diff] [review]
with both files....

Approved for 1.9.1.3 and 1.9.0.14, a=dveditz for release-drivers

Comment 12

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/194b881050ac

mozilla/netwerk/streamconv/converters/nsBinHexDecoder.cpp 	1.22
mozilla/netwerk/streamconv/converters/nsBinHexDecoder.h 	1.6

Comment 13

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

Joel or Aakash, can you verify that this is fixed in 1.9.1 now?

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 version

Comment 15

[Daniel Veditz [:dveditz]]

Checking in netwerk/streamconv/converters/nsBinHexDecoder.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsBinHexDecoder.cpp,v  <--  nsBinHexDecoder.cpp
new revision: 1.20.24.1; previous revision: 1.20
done
Checking in netwerk/streamconv/converters/nsBinHexDecoder.h;
/cvsroot/mozilla/netwerk/streamconv/converters/nsBinHexDecoder.h,v  <--  nsBinHexDecoder.h
new revision: 1.4.28.1; previous revision: 1.4
done