Closed Bug 1274509 Opened 4 years ago Closed 4 years ago

Do nsHttpRequestHead::VisitHeaders outside lock

Categories

(Core :: Networking: HTTP, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-active])

Attachments

(2 files, 2 obsolete files)

During nsHttpRequestHead::VisitHeaders nsHttpRequestHead is lock. This function is expose to outside of the internal gecko code. If any element of nsHttpRequestHead is access during VisitHeaders() call, it will deadlock. 

Maybe I am to cautious, but I was thinking that we should change this.
Whiteboard: [necko-active]
Attached patch bug_1274509_v1.patch (obsolete) — Splinter Review
Attachment #8754753 - Flags: review?(mcmanus)
Comment on attachment 8754753 [details] [diff] [review]
bug_1274509_v1.patch

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

if the headers are copied then the visitor can't modify what the channel is using.. that's a major change, right?

maybe using PRMonitor (which you would probably need write a small autoclass for) instead of mutex would do the trick. They are re-entrant on the same thread.. that might be a little iffy, but it seems to me the problem we are chasing is due to cross thread interaction.

::: netwerk/protocol/http/nsHttpRequestHead.h
@@ +40,5 @@
>      void SetRequestURI(const nsCSubstring &s);
>      void SetPath(const nsCSubstring &s);
>      uint32_t HeaderCount();
>  
> +    // This functions is going to copy headers under a lock and intereate

s/functions/function
Attachment #8754753 - Flags: review?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #4)
> Comment on attachment 8754753 [details] [diff] [review]
> bug_1274509_v1.patch
> 
> Review of attachment 8754753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> if the headers are copied then the visitor can't modify what the channel is
> using.. that's a major change, right?
> 

The visitor was getting only a copy of two strings: header and value, so changing them have not had and will not have(with this patch) any effect. In docs there is a warning that SetHeader should not be used and hopefully nobody uses because VisitHeader is an for loop through header array and deleting a header would cause trouble. With this patch calling SetHeader will be fine. 

> maybe using PRMonitor (which you would probably need write a small autoclass
> for) instead of mutex would do the trick. They are re-entrant on the same
> thread.. that might be a little iffy, but it seems to me the problem we are
> chasing is due to cross thread interaction.
> 

I knew there is something - PRMonitor, but I could not find it today :)
I will change it to PRMonitor.


> ::: netwerk/protocol/http/nsHttpRequestHead.h
> @@ +40,5 @@
> >      void SetRequestURI(const nsCSubstring &s);
> >      void SetPath(const nsCSubstring &s);
> >      uint32_t HeaderCount();
> >  
> > +    // This functions is going to copy headers under a lock and intereate
> 
> s/functions/function
Attached patch bug_1274509_v2.patch (obsolete) — Splinter Review
I have changed our behavior now to throw an error if someone tries to SetHeader while in VisitHeaders.

We will need to update docs as well.
Attachment #8754753 - Attachment is obsolete: true
Attachment #8755298 - Flags: review?(mcmanus)
Attachment #8755298 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Attachment #8755298 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8755821 - Flags: review+
Keywords: checkin-needed
For dev-doc-needed: I have only change that setHeaders, referrer(just setter) and setReferrerWithPolicy will return an error if called during VisitHeaders.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/697e260a0fd8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Documented this here:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel

Dragana, can you please have a look again whether the descriptions are correct?

Sebastian
Flags: needinfo?(dd.mozilla)
(In reply to Sebastian Zartner [:sebo] from comment #16)
> Documented this here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel
> 
> Dragana, can you please have a look again whether the descriptions are
> correct?
> 
> Sebastian


Thanks.

As part of visitRequestHeaders() description there is a warning:
"Warning: Calling setRequestHeader() while visiting request headers has undefined behavior. Don't do it!"

can you please extend it that setRequestHeader(), setReferrerWithPolicy(), setEmptyRequestHeader() and setting referrer attribute will return error starting with gecko49.
Flags: needinfo?(dd.mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #17)
> (In reply to Sebastian Zartner [:sebo] from comment #16)
> > Documented this here:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIHttpChannel
> > 
> > Dragana, can you please have a look again whether the descriptions are
> > correct?
> > 
> > Sebastian
> 
> 
> Thanks.
> 
> As part of visitRequestHeaders() description there is a warning:
> "Warning: Calling setRequestHeader() while visiting request headers has
> undefined behavior. Don't do it!"
> 
> can you please extend it that setRequestHeader(), setReferrerWithPolicy(),
> setEmptyRequestHeader() and setting referrer attribute will return error
> starting with gecko49.

Done. Thank you!

Sebastian
Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch

Approval Request Comment
[Feature/regressing bug #]: neede for uplift of bug 1247982
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low risk this patch only changes Mutex to ReentrantMonitor
[String/UUID change made/needed]:

the bug changes an idl - setting header and setting referrer during VisitRequestHeader was an unsafe operation, really hope no one did it, now it is going to return an error

Order of uplift:
bug 1247982,
bug 1269738,
this patch
Attachment #8755821 - Flags: approval-mozilla-aurora?
Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch

needs a rebase :)
Attachment #8755821 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/regressing bug #]: needed for uplift of bug 1247982
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low risk this patch only changes Mutex to ReentrantMonitor
[String/UUID change made/needed]:

the bug changes an idl - setting header and setting referrer during VisitRequestHeader was an unsafe operation, really hope no one did it, now it is going to return an error

Order of uplift:
bug 1247982,
bug 1269738,
this patch
Attachment #8759934 - Flags: approval-mozilla-aurora?
Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch

Switching the flag so it's for 48
Attachment #8759934 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch


Take it for the bug 1247982
Should be in 48 beta 3
Attachment #8759934 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
:sebo, may I ask you to update doc for this bug. It was uplifted to 48. Only requestHeader part was uplifted, the response header change has not been uplifted.

Thank you.
Flags: needinfo?(sebastianzartner)
I've updated the doc. Please verify that it's correct.

Sebastian
Flags: needinfo?(sebastianzartner) → needinfo?(dd.mozilla)
(In reply to Sebastian Zartner [:sebo] from comment #26)
> I've updated the doc. Please verify that it's correct.
> 
> Sebastian

Looks good, thanks!!!
Flags: needinfo?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.