Last Comment Bug 508074 - nsBinHexDecoder should use an nsCString for mName
: nsBinHexDecoder should use an nsCString for mName
Status: RESOLVED FIXED
[sg:critical?]
: crash, fixed1.8.1.24, fixed1.9.0.14
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: ARM Maemo
: -- critical (vote)
: ---
Assigned To: timeless
:
: Patrick McManus [:mcmanus]
Mentors:
http://download.adobe.com/pub/adobe/g...
Depends on: 543497
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-03 11:42 PDT by timeless
Modified: 2015-10-16 11:40 PDT (History)
12 users (show)
dveditz: blocking1.9.0.14+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.next?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.3+
.3-fixed


Attachments
patch (1.83 KB, patch)
2009-08-03 11:51 PDT, timeless
no flags Details | Diff | Splinter Review
mq header .... (1.86 KB, patch)
2009-08-03 11:57 PDT, timeless
no flags Details | Diff | Splinter Review
i need dinner (1.87 KB, patch)
2009-08-03 12:02 PDT, timeless
cbiesinger: review-
Details | Diff | Splinter Review
using an nsCString (3.84 KB, patch)
2009-08-04 03:31 PDT, timeless
cbiesinger: review+
Details | Diff | Splinter Review
with basic handling (2.56 KB, patch)
2009-08-05 08:04 PDT, timeless
no flags Details | Diff | Splinter Review
with both files.... (4.00 KB, patch)
2009-08-05 08:08 PDT, timeless
cbiesinger: review+
dveditz: approval1.9.1.3+
dveditz: approval1.9.0.14+
Details | Diff | Splinter Review
1.8 version (6.14 KB, patch)
2009-08-20 01:24 PDT, Martin Stránský
no flags Details | Diff | Splinter Review

Description timeless 2009-08-03 11:42:44 PDT
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 2009-08-03 11:51:06 PDT
Created attachment 392291 [details] [diff] [review]
patch
Comment 2 timeless 2009-08-03 11:57:11 PDT
Created attachment 392292 [details] [diff] [review]
mq header ....
Comment 3 timeless 2009-08-03 12:02:47 PDT
Created attachment 392294 [details] [diff] [review]
i need dinner
Comment 4 timeless 2009-08-04 03:31:31 PDT
Created attachment 392465 [details] [diff] [review]
using an nsCString

this patch requires the patch from bug 508229.
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-04 05:15:11 PDT
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 2009-08-05 08:04:56 PDT
Created attachment 392719 [details] [diff] [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).
Comment 7 timeless 2009-08-05 08:08:38 PDT
Created attachment 392720 [details] [diff] [review]
with both files....
Comment 9 Daniel Veditz [:dveditz] 2009-08-05 17:29:09 PDT
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:] 2009-08-06 09:45:30 PDT
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] 2009-08-07 12:34:14 PDT
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 2009-08-12 01:15:40 PDT
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] 2009-08-19 13:54:01 PDT
Joel or Aakash, can you verify that this is fixed in 1.9.1 now?
Comment 14 Martin Stránský 2009-08-20 01:24:31 PDT
Created attachment 395538 [details] [diff] [review]
1.8 version
Comment 15 Daniel Veditz [:dveditz] 2010-02-24 21:49:37 PST
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

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