Bugzilla@Mozilla – Bug 508074
nsBinHexDecoder should use an nsCString for mName
Last modified: 2011-03-25 13:54:23 PDT
Summon comment box
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
Created attachment 392291 [details] [review] patch
Created attachment 392292 [details] [review] mq header ....
Created attachment 392294 [details] [review] i need dinner
Created attachment 392465 [details] [review] using an nsCString this patch requires the patch from bug 508229.
Comment on attachment 392465 [details] [review] using an nsCString + mName.SetLength(c & 63); break; perhaps check that this succeeded: if (mName.Length() != (c & 63)) { // report error }
Created attachment 392719 [details] [review] 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).
Created attachment 392720 [details] [review] with both files....
http://hg.mozilla.org/mozilla-central/rev/f433451f8c40
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.
jmaher, I can save the page but not test that it actually reproduces the problem without a machine. Can you take a look?
Comment on attachment 392720 [details] [review] with both files.... Approved for 1.9.1.3 and 1.9.0.14, a=dveditz for release-drivers
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
Joel or Aakash, can you verify that this is fixed in 1.9.1 now?
Created attachment 395538 [details] [review] 1.8 version
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