Do nsHttpRequestHead::VisitHeaders outside lock

RESOLVED FIXED in Firefox 48

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

({dev-doc-complete})

48 Branch
mozilla49
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

2 years ago
Whiteboard: [necko-active]
(Assignee)

Comment 3

2 years ago
Created attachment 8754753 [details] [diff] [review]
bug_1274509_v1.patch
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)
(Assignee)

Comment 5

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

Comment 7

2 years ago
Created attachment 8755298 [details] [diff] [review]
bug_1274509_v2.patch

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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=28532193&repo=mozilla-inbound
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 12

2 years ago
Created attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch
Attachment #8755298 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8755821 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

2 years ago
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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/697e260a0fd8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
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)
(Assignee)

Comment 17

2 years ago
(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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 19

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

Comment 20

2 years ago
Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch

needs a rebase :)
Attachment #8755821 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

2 years ago
Created attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch


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?
status-firefox48: --- → affected
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+

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/52db915c585a
status-firefox48: affected → fixed
(Assignee)

Comment 25

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

Comment 27

2 years ago
(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.