Closed
Bug 869725
Opened 11 years ago
Closed 11 years ago
support shoutcast streams
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: rillian, Assigned: biggyspender)
References
()
Details
Attachments
(1 file, 1 obsolete file)
3.33 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
In bug 862088, it is requested that we support shoutcast mp3-over-http streams in the HTML <audio> tag. Two known issues: By default, the shoutcast server sniffs for 'Mozilla' in the user-agent and returns an html index page for the stream rather than the live audio/mpeg data. Not our bug, and this behaviour can be disabled by appending a semicolon (';') to the request url. The audio element sees the request, even with the semicolon as Content-type: text/plain. As per the HTML spec, we reject that as not an audio stream.
Reporter | ||
Comment 1•11 years ago
|
||
One theory is that shoutcast returns 'ICY 200 OK' which is causing our network stack to return text/plain instead of the supplied content-type. Looking at a packet dump, it does look like the server is sending 'content-type:audio/mpeg' in the response.
Reporter | ||
Comment 2•11 years ago
|
||
This is forked from https://bugzilla.mozilla.org/show_bug.cgi?id=862088#c21
Assignee | ||
Comment 3•11 years ago
|
||
I've been poking around at this. Here's what happens: - http request issued to ShoutCast server with path /;foo.mp3 (including a semi-colon in the path is the only way to solicit mp3 payload from SC) - SC replies with status line ICY 200 OK followed by HTTP/1.0-like headers - because this isn't an HTTP status line, nsHttpTransaction::LocateHttpStart fails to locate the status line - leads to nsHttpResponseHead::ParseVersion assuming HTTP/0.9 - as most streams are on non-default port, nsHttpChannel::CallOnStartRequest forces "text/plain" as mitigation of this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=667907 - as a result of being treated as HTTP/0.9, headers are passed on as part of the content body with content type of "text/plain" - audio refuses to play due to bad content-type I've patched my build such that nsHttpTransaction::LocateHttpStart now finds ICY in exactly the same fashion that HTTP/1. is found. nsHttpResponseHead::ParseVersion now also picks up an ICY status and in this case, assumes NS_HTTP_VERSION_1_0 instead of NS_HTTP_VERSION_0_9 . This allows for correct parsing of the subsequent headers, and most importantly, passes on the correct content-type to audio, with a properly aligned content body. With these changes, Shoutcast audio plays correctly and Ralph's example (http://people.mozilla.org/~rgiles/2013/shoutcast.html) shows "Ready to play" and plays perfectly (using a working stream, the one in the example is no longer broadcasting). I'm completely new to moz development. I don't know if it's wise to approach this in such a way. Given https://bugzilla.mozilla.org/show_bug.cgi?id=667907, there doesn't seem to be much scope for taking this on at a higher level. Anyone care to comment?
Reporter | ||
Comment 4•11 years ago
|
||
Thanks for the analysis. It seems like we have two ways to resolve this bug. We can update the network stack to accept the ICY 200 response, or we can force players to go through the WebAudio api, using XHR and managing the buffering themselves. The later involves CORS restrictions in addition to the added code complexity. Since the whole point with shoutcast support is that the server isn't going to to be updated, that may be a significant additional restriction. How interested are you in being able to play general streams? If you want to former to happen, you should attach your patch here and try to get it through the review process. It's not obvious to me that accepting 'ICY' as well as 'HTTP/1' introduces significant new XSS risk, but we'll have to have that discussion. The way to make that happen is to suggest a specific code change.
Assignee | ||
Comment 5•11 years ago
|
||
I have good visibility of the adoption of new versions of Shoutcast server and it's extremely slow, with the vast majority of broadcasters still on "SHOUTcast Distributed Network Audio Server/xxxx v1.9.8". Adoption of Shoutcast 2 is just short of 4% after about a year, but I'm not sure that v2 offers any significant change over v1 from the perspective of this issue. It has taken 3-4 years for Flash policy file (crossdomain.xml) support to be added, so if I were to start making a noise about adding origin headers, we might expect similar timeframes. I suspect that the ICY header is so baked-in to the Winamp/Shoutcast stack that it will never change, no matter how appealing a standard HTTP status-line might be. In short, the current situation isn't likely to change for a long time. IMO, the former approach is the only realistic means of adding support. There's already special-casing of a non-standard status line in order to support "SmarterTools/2.0.3974.16813" server ( https://bugzilla.mozilla.org/show_bug.cgi?id=635892 ), so this wouldn't be completely alien. Once I've made sure I'm up to speed on the submission process, I'll tidy up my changes and attach the patch here.
Reporter | ||
Comment 6•11 years ago
|
||
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch has an overview of the process. I'm happy to help along the way.
Comment 7•11 years ago
|
||
I added a secreview flag cc:dveditz to take a look at this; at the very least we can get someone assigned to review it on Wednesday.
Flags: sec-review?(dveditz)
Whiteboard: [triage needed]
Reporter | ||
Comment 8•11 years ago
|
||
Thanks, Yvan. Spender, can you post a provisional patch before then?
Assignee | ||
Comment 9•11 years ago
|
||
Yvan, Ralph... That shouldn't be a problem (qualified with a small amount of uncertainly because this morning my line started suffering from fairly bad packet loss that isn't scheduled to be fixed until Thursday. I suppose I can always fall back on my phone). Thanks.
changing flags and tags so this shows up in Security triage on Wed
Flags: sec-review?(dveditz) → sec-review?
Whiteboard: [triage needed]
Updated•11 years ago
|
Flags: needinfo?(dveditz)
Comment 11•11 years ago
|
||
I'm not sure what info is specifically wanted, but from a security POV I'm fine with treating "ICY 200 OK" as synonymous to "HTTP 200 OK" and treating the headers as http 1.0
Flags: needinfo?(dveditz)
Reporter | ||
Comment 12•11 years ago
|
||
Thanks, Daniel. That's the info we wanted. Seemed reasonable to me, but I don't have much experience with XSS. From the code, I think we'd actually be treating 'ICY' as synonymous with 'HTTP' with the rest of the line parsed as if it was http 1.0. We'll see when spender posts a patch.
Assignee | ||
Comment 13•11 years ago
|
||
Bug 869725 - Changes to HTTP status line detection and parsing to allow AOL/Nullsoft ShoutCast headers (e.g. ICY 200 OK)
Assignee | ||
Comment 14•11 years ago
|
||
Updated my profile with my real name. Apologies for confusion. -spender
Reporter | ||
Comment 15•11 years ago
|
||
Comment on attachment 749459 [details] [diff] [review] preliminary patch to allow ICY response and treat like HTTP/1.0 Review of attachment 749459 [details] [diff] [review]: ----------------------------------------------------------------- Some style nits. Otherwise looks reasonable to me. ::: netwerk/protocol/http/nsHttpResponseHead.cpp @@ +581,5 @@ > + str += 4; > + } > + else{ > + //ICY is HTTP/1.0-like, so below we'll assume HTTP/1.0 because the next char isn't '/' > + if (PL_strncasecmp(str, "ICY", 3) == 0) { Can you make this an: if (..."HTTP"...) { ... } else if (...."ICY"...) { ... } else { ... } I think that would be easier to read. @@ +593,2 @@ > } > + Please remove the trailing whitespace characters here. ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +971,5 @@ > nsHttpTransaction::LocateHttpStart(char *buf, uint32_t len, > bool aAllowPartialMatch) > { > NS_ASSERTION(!aAllowPartialMatch || mLineBuf.IsEmpty(), "ouch"); > + Whitespace. @@ +978,5 @@ > static const char HTTPHeader[] = "HTTP/1."; > static const uint32_t HTTPHeaderLen = sizeof(HTTPHeader) - 1; > static const char HTTP2Header[] = "HTTP/2.0"; > static const uint32_t HTTP2HeaderLen = sizeof(HTTP2Header) - 1; > Whitespace. @@ +1028,5 @@ > return buf; > } > > + // Treat ICY (AOL/Nullsoft ShoutCast) non-standard header in same > + // fashion as HTTP/2.0 above. This will cause status line to be interpretted Please remove trailing whitespace in the comment.
Reporter | ||
Comment 16•11 years ago
|
||
Oops. You don't have to remove trailing whitespace on lines you don't otherwise change. I was just trying to avoid adding more.
Comment 17•11 years ago
|
||
When rgiles is happy with the patch don't forget that since it's in /netwerk it needs r+ from actual networking guys before it lands. jduell and mcmanus are good choices or they could suggest others if they're swamped.
Comment 18•11 years ago
|
||
Comment on attachment 749459 [details] [diff] [review] preliminary patch to allow ICY response and treat like HTTP/1.0 Review of attachment 749459 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpResponseHead.cpp @@ +581,5 @@ > + str += 4; > + } > + else{ > + //ICY is HTTP/1.0-like, so below we'll assume HTTP/1.0 because the next char isn't '/' > + if (PL_strncasecmp(str, "ICY", 3) == 0) { > if (PL_strncasecmp(str, "ICY", 3) == 0) { What if the line starts ICYFAKE? Since the next char is not '/' this would be accepted. Hm, I guess the old code has the same problem with HTTPNOTREALLY. If we encounter ICY/1.1 we'll treat it as HTTP/1.1--might be OK but should someone ever create an Icecast x.y server that's different than the corresponding HTTP/x.y we might treat it incorrectly. Probably safest to look for "ICY " and make sure the expected space is there. You can't then drop into common slash-checking code, but if you re-order as suggested by Ralph you won't be doing that anyways. ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +978,1 @@ > static const char HTTPHeader[] = "HTTP/1."; ObNit: since this is the HTTP protocol it seems odd to lead a section with ICY constants. I'd prefer sticking this on the bottom of the list, and it wouldn't hurt to add a comment saying "// Icecast is HTTP/1.0" or similar. I know you've got the same comment 60 lines down but it can't hurt to leave reminders since this is not expected in normal HTTP code.
Comment 19•11 years ago
|
||
Comment on attachment 749459 [details] [diff] [review] preliminary patch to allow ICY response and treat like HTTP/1.0 Review of attachment 749459 [details] [diff] [review]: ----------------------------------------------------------------- Chris, thanks for the patch! This is a totally reasonable thing to do if ICY really is HTTP/1.0. Searching around I didn't find any differences other than the version string, but if there do turn out to be differences in framing or anything like that then we will probably need to find a different approach. But for now, this seems like a good thing to try. you have a lot of good review comments already, please consider those as part of my comments too. your patch includes tabs where there should be spaces, please make sure to fix that up. Spaces only. please assign the r? to me when you're ready. ::: netwerk/protocol/http/nsHttpResponseHead.cpp @@ +572,2 @@ > void > nsHttpResponseHead::ParseVersion(const char *str) I would prefer this part of the diff just looks like the below (if it works for you) diff --git a/netwerk/protocol/http/nsHttpResponseHead.cpp b/netwerk/protocol/http/nsHttpResponseHead.cpp --- a/netwerk/protocol/http/nsHttpResponseHead.cpp +++ b/netwerk/protocol/http/nsHttpResponseHead.cpp @@ -573,16 +573,22 @@ void nsHttpResponseHead::ParseVersion(const char *str) { // Parse HTTP-Version:: "HTTP" "/" 1*DIGIT "." 1*DIGIT LOG(("nsHttpResponseHead::ParseVersion [version=%s]\n", str)); // make sure we have HTTP at the beginning if (PL_strncasecmp(str, "HTTP", 4) != 0) { + // Check for ICY (AOL/Nullsoft ShoutCast) and map to 1.0 + if (!PL_strncasecmp(str, "ICY ", 4)) { + LOG(("Treating ICY as HTTP 1.0\n")); + mVersion = NS_HTTP_VERSION_1_0; + return; + } LOG(("looks like a HTTP/0.9 response\n")); mVersion = NS_HTTP_VERSION_0_9; return; } str += 4; if (*str != '/') { LOG(("server did not send a version number; assuming HTTP/1.0\n")); ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +972,5 @@ > bool aAllowPartialMatch) > { > NS_ASSERTION(!aAllowPartialMatch || mLineBuf.IsEmpty(), "ouch"); > + > + static const char ICYHeader[] = "ICY"; I think we can make this "ICY " to further target it.
Attachment #749459 -
Flags: feedback+
Updated•11 years ago
|
Flags: sec-review? → sec-review?(dveditz)
Assignee | ||
Comment 20•11 years ago
|
||
Removed tabs/trailing spaces/excessive line lengths, and implemented code suggestions and comments.
Attachment #749459 -
Attachment is obsolete: true
Attachment #750140 -
Flags: review?(mcmanus)
Updated•11 years ago
|
Attachment #750140 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Any further action required by me?
Comment 22•11 years ago
|
||
(In reply to Chris Sperry from comment #21) > Any further action required by me? Not immediately. I'll push the push to our Try Server now, where we can make sure it builds on all platforms and doesn't break any existing automated tests: https://tbpl.mozilla.org/?tree=Try&rev=c8a02c2f4df4 Assuming that goes well, one of us with commit access can then land the patch on mozilla-inbound, and within a day or two it will be in Firefox nightly builds! It would also be good to have an automated test for this bug to make sure it remains fixed. If that's something you're interested in, you can write a test and attach it as a separate patch on this bug or in a new bug. The patch in bug 667907 might give you some idea of how to write it; see also: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests
Comment 23•11 years ago
|
||
The Try run passed, so I landed this on inbound. Thanks again! https://hg.mozilla.org/integration/mozilla-inbound/rev/0dab00052bf7
Assignee: nobody → biggyspender
Status: NEW → ASSIGNED
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0dab00052bf7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•11 years ago
|
Flags: sec-review?(dveditz) → sec-review+
We should backport this patch to b2g18 to enable Shoutcast playback on FirefoxOS 1.1. This will help get Internet radio apps working on FirefoxOS. The patch certainly looks harmless enough.
blocking-b2g: --- → leo?
Comment 26•11 years ago
|
||
Triage - new feature, bumping to koi? given current leo stage. Partner also cannot add/modify feature to the project now as it's near the end of its testing.
blocking-b2g: leo? → koi?
Comment 27•11 years ago
|
||
Can we leo? to see if other partners are willing to take this?
Updated•11 years ago
|
blocking-b2g: koi? → leo?
Comment 28•11 years ago
|
||
Thanks, everybody, for this fix! Awesome. This just saved us, for an extension we're developing. Thanks a lot.
Comment 29•11 years ago
|
||
Filed RFC bug 908902 for the metadata.
Comment 30•11 years ago
|
||
(In reply to Chris Lee [:clee] from comment #27) > Can we leo? to see if other partners are willing to take this? This is a feature, which it's definitely too late to take at this point for 1.1.
Comment 31•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #30) > (In reply to Chris Lee [:clee] from comment #27) > > Can we leo? to see if other partners are willing to take this? > > This is a feature, which it's definitely too late to take at this point for > 1.1. ^ This. Late features -> late release. If Shoutcast wasn't on the list before this, the priority certainly seems too low to risk at this extremely late point in the release cycle. It's already landed, which means it's already included in 1.2.
blocking-b2g: leo? → -
You need to log in
before you can comment on or make changes to this bug.
Description
•