Closed Bug 631967 Opened 9 years ago Closed 9 years ago

Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID

Categories

(Core :: Networking: HTTP, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: jdm, Assigned: edsrzf)

Details

(Whiteboard: [good first bug][mentor=jdm][inbound])

Attachments

(1 file, 1 obsolete file)

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();
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.
Whiteboard: [good first bug][mentor=jdm]
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
Attached patch Proposed patch (obsolete) — Splinter Review
I've chosen this bug for my first patch. Hopefully I got it right. :)
Attachment #547345 - Flags: review?(josh)
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.
Attachment #547345 - Flags: review?(josh) → review?(honzab.moz)
Attached patch Patch 2Splinter Review
Patch 2 attached; hopefully in the correct format this time.
Attachment #547345 - Attachment is obsolete: true
Attachment #547613 - Flags: review?(honzab.moz)
Attachment #547345 - Flags: review?(honzab.moz)
Comment on attachment 547613 [details] [diff] [review]
Patch 2

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

r=honzab
Attachment #547613 - Flags: review?(honzab.moz) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/9dda487bc910

Thanks Evan!
Assignee: nobody → edsrzf
Whiteboard: [good first bug][mentor=jdm] → [good first bug][mentor=jdm][inbound]
http://hg.mozilla.org/mozilla-central/rev/9dda487bc910
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.