HPKP and HSTS buildbot scripts did not run

RESOLVED FIXED in Firefox 36

Status

()

Core
Networking
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mmc, Assigned: mcmanus)

Tracking

Trunk
mozilla37
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(3 attachments)

I don't see any changes to m-c between Friday at 5PT and Monday morning.
Flags: needinfo?(coop)
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/mozilla-central-linux64-periodicupdate-bm84-build1-build0.txt.gz

(To find them yourself starting from http://ftp.mozilla.org/pub/mozilla.org/firefox/, you just have to remember two things, "it's a tinderbox-build rather than a nightly or candidate" and "they run on Linux64".)

As to *why* it segfaulted while running... dunno.
Thanks philor. Hmm, that sounds familiar, kind of like https://bugzilla.mozilla.org/show_bug.cgi?id=1083451.
Flags: needinfo?(coop) → needinfo?(dkeeler)
Created attachment 8537443 [details]
1111875.js

What I'm seeing is if a site responds with a redirect and includes a content-length: 0 header but actually sends content and the channel has a notificationCallbacks object that prevents redirects, we hit the assertion at http://hg.mozilla.org/mozilla-central/annotate/a3030140d5df/netwerk/protocol/http/nsHttpChannel.cpp#l5650

Here's an xpcshell script that illustrates the issue.
Flags: needinfo?(dkeeler)
Created attachment 8537444 [details]
output.log

Here's an nsHttp NSPR log, for good measure.
Patrick, any ideas as to what's wrong here? (see in particular comment 3 - thanks)
Flags: needinfo?(mcmanus)
(Assignee)

Comment 6

3 years ago
first, what an awesomely constructed bugzilla.

next, the bitmex transaction is illegal - 0 bytes of content-length and then more bytes of body. So if this is somehow part of our infrastructure we need to get them to fix things too - throwing an error on the channel would be reasonable. (and we've tried to do that a couple times with underflows.)

anyhow, the moz_assert I trip with your testcase is actually at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5813 .. the one you cite in comment 3 is just a ns_warning.. this one at 5813 should be a warning too - we intentionally are accepting that data for now, so asserting is wrong.

I'd be happy to r+ the change for efficiency.

last, this should only impact debug builds.. I'm not sure that squares with the title of this bug. does it?
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #6)
> first, what an awesomely constructed bugzilla.
> 
> next, the bitmex transaction is illegal - 0 bytes of content-length and then
> more bytes of body. So if this is somehow part of our infrastructure we need
> to get them to fix things too - throwing an error on the channel would be
> reasonable. (and we've tried to do that a couple times with underflows.)
> 
> anyhow, the moz_assert I trip with your testcase is actually at
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#5813 .. the one you cite in comment 3 is just a
> ns_warning.. this one at 5813 should be a warning too - we intentionally are
> accepting that data for now, so asserting is wrong.
> 
> I'd be happy to r+ the change for efficiency.
> 
> last, this should only impact debug builds.. I'm not sure that squares with
> the title of this bug. does it?

I guess buildbot runs debug xpcshell, which I didn't realize. Sorry for the bad comment 0 -- this is affecting weekly updates for https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/StaticHPKPins.h.
(Assignee)

Comment 8

3 years ago
Created attachment 8537576 [details] [diff] [review]
[PATCH]  - remove assertion on http size input error
Attachment #8537576 - Flags: review?(dkeeler)
(Assignee)

Updated

3 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 years ago
the other way to workaround this would be to run an opt xpcshell
Component: Release Automation → General Automation
QA Contact: bhearsum → catlee
Comment on attachment 8537576 [details] [diff] [review]
[PATCH]  - remove assertion on http size input error

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

Thanks, Pat! I verified this was the assertion keeler's test was hitting, too.
Attachment #8537576 - Flags: review+
Comment on attachment 8537576 [details] [diff] [review]
[PATCH]  - remove assertion on http size input error

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

LGTM - thanks!
Attachment #8537576 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/8b71a52d7730
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8537576 [details] [diff] [review]
[PATCH]  - remove assertion on http size input error

Approval Request Comment
[Feature/regressing bug #]: bitmex.com sending back malformed response
[User impact if declined]: buildbot cannot run on Aurora to update HSTS and HPKP files
[Describe test coverage new/current, TBPL]: m-i
[Risks and why]: low, buildbot is already segfaulting (see comment 1)
[String/UUID change made/needed]: none
Attachment #8537576 - Flags: approval-mozilla-aurora?
(In reply to Patrick McManus [:mcmanus] from comment #9)
> the other way to workaround this would be to run an opt xpcshell

Thanks, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1113195 for this.
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8537576 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a8f7033e1897
status-firefox36: affected → fixed
Component: General Automation → Networking
Product: Release Engineering → Core
QA Contact: catlee
Target Milestone: --- → mozilla37
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.