Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 869725 - support shoutcast streams
: support shoutcast streams
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla24
Assigned To: Chris Sperry
: Maire Reavy [:mreavy]
Depends on:
Blocks: 903540 913236
  Show dependency treegraph
Reported: 2013-05-07 17:04 PDT by Ralph Giles (:rillian) needinfo me
Modified: 2013-09-05 15:43 PDT (History)
14 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

preliminary patch to allow ICY response and treat like HTTP/1.0 (3.48 KB, patch)
2013-05-14 13:40 PDT, Chris Sperry
mcmanus: feedback+
Details | Diff | Splinter Review
Tidied patch for allowing ICY response (3.33 KB, patch)
2013-05-15 16:09 PDT, Chris Sperry
mcmanus: review+
Details | Diff | Splinter Review

Description Ralph Giles (:rillian) needinfo me 2013-05-07 17:04:26 PDT
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.
Comment 1 Ralph Giles (:rillian) needinfo me 2013-05-07 17:07:53 PDT
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.
Comment 2 Ralph Giles (:rillian) needinfo me 2013-05-07 17:09:15 PDT
This is forked from
Comment 3 Chris Sperry 2013-05-09 09:26:17 PDT
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:
- 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 ( 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, there doesn't seem to be much scope for taking this on at a higher level. Anyone care to comment?
Comment 4 Ralph Giles (:rillian) needinfo me 2013-05-10 12:54:34 PDT
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.
Comment 5 Chris Sperry 2013-05-10 14:03:25 PDT
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 ( ), 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.
Comment 6 Ralph Giles (:rillian) needinfo me 2013-05-10 15:23:45 PDT has an overview of the process. I'm happy to help along the way.
Comment 7 Yvan Boily [:ygjb][:yvan] 2013-05-13 08:58:36 PDT
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.
Comment 8 Ralph Giles (:rillian) needinfo me 2013-05-13 09:47:34 PDT
Thanks, Yvan. Spender, can you post a provisional patch before then?
Comment 9 Chris Sperry 2013-05-13 10:22:43 PDT
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.
Comment 10 Curtis Koenig [:curtisk-use]] 2013-05-13 10:55:25 PDT
changing flags and tags so this shows up in Security triage on Wed
Comment 11 Daniel Veditz [:dveditz] 2013-05-13 19:27:07 PDT
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
Comment 12 Ralph Giles (:rillian) needinfo me 2013-05-14 09:10:48 PDT
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.
Comment 13 Chris Sperry 2013-05-14 13:40:05 PDT
Created attachment 749459 [details] [diff] [review]
preliminary patch to allow ICY response and treat like HTTP/1.0

Bug 869725 - Changes to HTTP status line detection and parsing to allow AOL/Nullsoft ShoutCast headers (e.g. ICY 200 OK)
Comment 14 Chris Sperry 2013-05-14 13:47:32 PDT
Updated my profile with my real name. Apologies for confusion. -spender
Comment 15 Ralph Giles (:rillian) needinfo me 2013-05-14 15:44:53 PDT
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");
> +	


@@ +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;


@@ +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.
Comment 16 Ralph Giles (:rillian) needinfo me 2013-05-14 15:47:19 PDT
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 Daniel Veditz [:dveditz] 2013-05-14 20:07:43 PDT
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 Daniel Veditz [:dveditz] 2013-05-14 20:31:03 PDT
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 Patrick McManus [:mcmanus] 2013-05-15 06:50:21 PDT
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;
     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.
Comment 20 Chris Sperry 2013-05-15 16:09:56 PDT
Created attachment 750140 [details] [diff] [review]
Tidied patch for allowing ICY response

Removed tabs/trailing spaces/excessive line lengths, and implemented code suggestions and comments.
Comment 21 Chris Sperry 2013-05-16 06:06:01 PDT
Any further action required by me?
Comment 22 Matt Brubeck (:mbrubeck) 2013-05-16 07:19:55 PDT
(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:

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:
Comment 23 Matt Brubeck (:mbrubeck) 2013-05-16 12:26:02 PDT
The Try run passed, so I landed this on inbound.  Thanks again!
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-05-16 17:39:05 PDT
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-08-08 17:29:42 PDT
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.
Comment 26 Wayne Chang [:wchang] 2013-08-09 01:18:07 PDT
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.
Comment 27 Chris Lee [:clee] 2013-08-14 13:03:50 PDT
Can we leo? to see if other partners are willing to take this?
Comment 28 Ben Bucksch (:BenB) 2013-08-23 15:13:11 PDT
Thanks, everybody, for this fix! Awesome. This just saved us, for an extension we're developing. Thanks a lot.
Comment 29 Ben Bucksch (:BenB) 2013-08-23 15:28:26 PDT
Filed RFC bug 908902 for the metadata.
Comment 30 Jason Smith [:jsmith] 2013-08-25 06:16:19 PDT
(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 Dietrich Ayala (:dietrich) 2013-08-26 08:16:47 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.