Last Comment Bug 631967 - Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID
: Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID
[good first bug][mentor=jdm][inbound]
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86 Linux
-- normal (vote)
: mozilla8
Assigned To: Evan Shaw
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2011-02-06 22:49 PST by Josh Matthews [:jdm]
Modified: 2011-08-03 02:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed patch (1.68 KB, patch)
2011-07-21 02:17 PDT, Evan Shaw
no flags Details | Diff | Splinter Review
Patch 2 (1.80 KB, patch)
2011-07-21 22:29 PDT, Evan Shaw
honzab.moz: review+
Details | Diff | Splinter Review

Description User image Josh Matthews [:jdm] 2011-02-06 22:49:19 PST
Either mResponseHead can be null in GetEntityID, or it can't, but these bits need to change one way or another:

>1203   const char* acceptRanges =
>1204       mResponseHead->PeekHeader(nsHttp::Accept_Ranges);
>1212   if (mResponseHead) {
>1213     size = mResponseHead->TotalEntitySize();
Comment 1 User image Josh Matthews [:jdm] 2011-05-30 11:49:05 PDT
This is a very simple bug to resolve. Whoever ends up working on it should get input from Jason or Honza on whether to add a null check or remove the existing one.
Comment 2 User image Honza Bambas (:mayhemer) 2011-06-10 08:24:34 PDT
After on OnStartRequest we have to return something, nevertheless we have the response head.

The change with acceptRanges has been added in bug 462707 [1].  Ehsan apparently forgot to add a null check, I would say.

We should move his code into the if block.

Comment 3 User image Evan Shaw 2011-07-21 02:17:58 PDT
Created attachment 547345 [details] [diff] [review]
Proposed patch

I've chosen this bug for my first patch. Hopefully I got it right. :)
Comment 4 User image Josh Matthews [:jdm] 2011-07-21 07:43:21 PDT
Comment on attachment 547345 [details] [diff] [review]
Proposed patch

Thanks very much for your patch, Evan! It looks right to me, but I will defer to Honza's review on this, since he is a peer of this code. However, if you could upload another version that follows the guidelines at then it will be easier to check in this patch once the review process is finished.
Comment 5 User image Evan Shaw 2011-07-21 22:29:13 PDT
Created attachment 547613 [details] [diff] [review]
Patch 2

Patch 2 attached; hopefully in the correct format this time.
Comment 6 User image Honza Bambas (:mayhemer) 2011-07-29 12:18:32 PDT
Comment on attachment 547613 [details] [diff] [review]
Patch 2

Review of attachment 547613 [details] [diff] [review]:

Comment 7 User image Josh Matthews [:jdm] 2011-08-02 11:30:20 PDT

Thanks Evan!
Comment 8 User image Marco Bonardo [::mak] 2011-08-03 02:18:09 PDT

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