Last Comment Bug 631967 - Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID
: Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID
Status: RESOLVED FIXED
[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
:
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 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 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 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.

[1] http://hg.mozilla.org/mozilla-central/rev/356e3865c629
Comment 3 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 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 https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f then it will be easier to check in this patch once the review process is finished.
Comment 5 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 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]:
-----------------------------------------------------------------

r=honzab
Comment 7 Josh Matthews [:jdm] 2011-08-02 11:30:20 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9dda487bc910

Thanks Evan!
Comment 8 Marco Bonardo [::mak] 2011-08-03 02:18:09 PDT
http://hg.mozilla.org/mozilla-central/rev/9dda487bc910

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