Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsBinHexDecoder should use an nsCString for mName

RESOLVED FIXED

Status

()

Core
Networking
--
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

({crash, fixed1.8.1.24, fixed1.9.0.14})

Trunk
ARM
Maemo
crash, fixed1.8.1.24, fixed1.9.0.14
Points:
---
Bug Flags:
blocking1.9.0.14 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
blocking1.8.0.next ?

Firefox Tracking Flags

(blocking1.9.1 .3+, status1.9.1 .3-fixed)

Details

(Whiteboard: [sg:critical?], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
Created attachment 392291 [details] [diff] [review]
patch
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #392291 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

8 years ago
Created attachment 392292 [details] [diff] [review]
mq header ....
Attachment #392291 - Attachment is obsolete: true
Attachment #392292 - Flags: review?(cbiesinger)
Attachment #392291 - Flags: review?(cbiesinger)
(Assignee)

Comment 3

8 years ago
Created attachment 392294 [details] [diff] [review]
i need dinner
Attachment #392292 - Attachment is obsolete: true
Attachment #392294 - Flags: review?(cbiesinger)
Attachment #392292 - Flags: review?(cbiesinger)
(Assignee)

Updated

8 years ago
Summary: Bad free in nsBinHexDecoder::~nsBinHexDecoder → nsBinHexDecoder should use an nsCString for mName
(Assignee)

Comment 4

8 years ago
Created attachment 392465 [details] [diff] [review]
using an nsCString

this patch requires the patch from bug 508229.
Attachment #392465 - Flags: review?(cbiesinger)
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
  }
Attachment #392465 - Flags: review?(cbiesinger) → review+
Attachment #392294 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 6

8 years ago
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).
Attachment #392465 - Attachment is obsolete: true
Attachment #392719 - Flags: review?(cbiesinger)
(Assignee)

Comment 7

8 years ago
Created attachment 392720 [details] [diff] [review]
with both files....
Attachment #392719 - Attachment is obsolete: true
Attachment #392720 - Flags: review?(cbiesinger)
Attachment #392719 - Flags: review?(cbiesinger)
Attachment #392720 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 8

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f433451f8c40
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
blocking1.9.1: --- → ?
status1.9.1: --- → wanted
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.14?
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next?
Keywords: testcase-wanted
Whiteboard: [sg:critical?]

Comment 10

8 years ago
jmaher, I can save the page but not test that it actually reproduces the problem without a machine. Can you take a look?
Flags: blocking1.9.0.14? → blocking1.9.0.14+
blocking1.9.1: ? → .3+
Attachment #392720 - Flags: approval1.9.1.3?
Attachment #392720 - Flags: approval1.9.0.14?
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
Attachment #392720 - Flags: approval1.9.1.3?
Attachment #392720 - Flags: approval1.9.1.3+
Attachment #392720 - Flags: approval1.9.0.14?
Attachment #392720 - Flags: approval1.9.0.14+
(Assignee)

Comment 12

8 years ago
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
status1.9.1: wanted → .3-fixed
Keywords: 4xp, fixed1.9.0.14
(Assignee)

Updated

8 years ago
Keywords: 4xp
Joel or Aakash, can you verify that this is fixed in 1.9.1 now?

Comment 14

8 years ago
Created attachment 395538 [details] [diff] [review]
1.8 version
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Depends on: 543497
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
Keywords: fixed1.8.1.24
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.