cajbir (:cajbir) Assignee
	Description • 12 years ago

We derive our own DataSource from the android base class to use Gecko's network and media cache code. Devices that customize the base class have a different memory layout and this causes crashes. 

This is the number one cause of the Android stagefright crashing bugs and it is time consuming to obtain devices and track down the modifications they've made in their closed source binary components to create workarounds. This is not scalable.

This bug is to implement an alternative solution. We plan to spawn a socket that acts as an HTTP proxy and use Android's DataSource::CreateFromURI to connect to that. This removes the need to derive from DataSource and should be compatible with device manufacturers changes.

	cajbir (:cajbir) Assignee
	Comment 1 • 11 years ago

This replaces our usage of deriving from Android's DataSource class for reading from our MediaResource to instead spawn a socket that the internal Android DataSource implementation connects to. That socket implements a mini-http server with just enough HTTP to satisfy DataSource. The server forwards requests to a MediaResource instance.

	cajbir (:cajbir) Assignee
	Comment 2 • 11 years ago

Try server: https://tbpl.mozilla.org/?tree=Try&rev=9f1bf3344d79

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 3 • 11 years ago

Comment on attachment 769544 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

Phew, lots of work here. I wish this existed when I came to first write the WMF backend!

I think you should get a networking guy to look at your proxy, and Sotoro to at least provide feedback on it.

I think you need a way to remove a MediaResource from the map in MediaResourceServer when the decoder shuts down. Otherwise you'll hold an owning reference that keeps every MediaResource alive until the MediaResourceServer is deleted when the MediaPluginHost singleton is deleted.

::: content/media/plugins/MediaResourceServer.cpp
@@ +22,5 @@
> +*/
> +template<typename CharT, class StreamType, class StringType>
> +nsresult
> +ReadCRLF (StreamType* aStream, nsLineBuffer<CharT> * aBuffer,
> +             StringType & aLine, bool *more)

not: aMore, and indentation.

@@ +24,5 @@
> +nsresult
> +ReadCRLF (StreamType* aStream, nsLineBuffer<CharT> * aBuffer,
> +             StringType & aLine, bool *more)
> +{
> +  CharT eolchar = 0; // the first eol char or 1 after \r\n or \n\r is found

Doesn't this only detect \r\n, not \n\r, i.e. this comment is wrong?

@@ +44,5 @@
> +
> +    /*
> +     * Walk the buffer looking for an end-of-line.
> +     * There are 3 cases to consider:
> +     *  1. the CR char is the last char in the buffer

How you handle the case where the \r\n spans two buffer boundaries? It seems you're only handling the case where the \r\n is entirely contained in the current buffer...

@@ +54,5 @@
> +    CharT* current = aBuffer->start;
> +    if (MOZ_LIKELY(eolchar == 0)) {
> +      for ( ; current < aBuffer->end-1; ++current) {
> +        if (*current == '\r' && *(current+1) == '\n') {
> +          eolchar = 1;

eolchar can only be non zero on this branch, which exits, so you don't need eolchar; the eolchar==0 checks elsewhere in this function will always be true.

@@ +99,5 @@
> +  NS_IMETHOD Run();
> +
> +  // Given the first line of an HTTP request, parse the URL requested and
> +  // return the MediaResource for that URL.
> +  nsRefPtr<MediaResource> GetMediaResource(nsCString const& aHTTPRequest);

s/nsRefPtr<MediaResource>/already_AddRefed<MediaResource>/

@@ +105,5 @@
> +  // Gracefully shutdown the thread and cleanup resources
> +  void Shutdown();
> +};
> +
> +nsRefPtr<MediaResource>

Won't this return-by-value, i.e. create a copy of the nsRefPtr, going through an addref/release cycle?

You should be returning an already_AddRefed<MediaResource>.

@@ +185,5 @@
> +  if (start > 0 && !resource->IsTransportSeekable()) {
> +    start = 0;
> +  }
> +
> +  if (start > 0) {

Shouldn't this be:
if (start != resource->Tell())

?

Otherwise if we have a read following a on after one that leaves the resource at a non-zero position, the read following on which won't start reading at zero as intended?

@@ +225,5 @@
> +             start, resource->GetLength() - 1, resource->GetLength());
> +    rv = mOutput->Write(b, strlen(b), &len);
> +  }
> +
> +  rv = mOutput->Write(response_end, strlen(response_end), &len);

Does the write fail if the other side closes connection? i.e. can you avoid doing unnecessary blocking-reads below if the write fails here (and above where you call Write())?

@@ +229,5 @@
> +  rv = mOutput->Write(response_end, strlen(response_end), &len);
> +  rv = mOutput->Flush();
> +
> +  // Read data from media resource
> +  rv = resource->Read(b, buffer_size, &len);

I think we should add MediaResource::ReadAt(char* buffer, uint32_t bufferLen, uint32_t* aOutBytesRead, int64_t offset), which allows us to seek+read in a safe way using the MediaCache's lock. We're having to code around this limitation in the WMF, OMX backends (and I had to in my mythical DirectShow backend too), so we may as well fix it properly here. Then you don't have to worry about concurrent reads and my comment about tell above; the read will be synchronized by the media cache.

@@ +244,5 @@
> +    // If the start position for the next read that we think we are at is
> +    // different from the actual position then we exit and cleanup all
> +    // the resources. This is due to some other thread using the MediaResource
> +    // to read or seek - probably due to the Android DataSource making
> +    // another connection for the same MediaResource and it's safe to exit this

How do you know that it's safe to abort the read if another read is attempted? If I recall what Sotaro told me correctly, libstagefright has two threads that do reads, one for decoding audio and one for decoding video, and both these can read simultaneously, that's why Sotaro added a lock around the MediaResource usage in Bug 874325. Then again libstagefright might have an abstraction layer like our MediaResource which allows reads from multiple threads but only makes one HTTP request at a time. It makes sense to me to only stop sending data down the pipe if the other side stops reading it.

@@ +274,5 @@
> +    Shutdown(nsIThread* aThread) : mThread(aThread) {}
> +    nsresult Run() MOZ_OVERRIDE { return mThread->Shutdown(); }
> +  };
> +
> +  nsCOMPtr<nsIRunnable> event = new Shutdown(NS_GetCurrentThread());

You can use ShutdownThreadEvent from http://mxr.mozilla.org/mozilla-central/source/content/media/VideoUtils.h#115

@@ +364,5 @@
> +  return NS_OK;
> +}
> +
> +/* static */
> +nsCOMPtr<MediaResourceServer>

already_AddRefed<MediaReesourceServer>

@@ +369,5 @@
> +MediaResourceServer::Start()
> +{
> +  nsCOMPtr<MediaResourceServer> server = new MediaResourceServer();
> +  NS_DispatchToMainThread(server, NS_DISPATCH_SYNC);
> +  return server;

return server.forget();

@@ +410,5 @@
> +  snprintf(buffer, sizeof(buffer), "http://127.0.0.1:%d", port >= 0 ? port : 0);
> +  return nsCString(buffer);
> +}
> +
> +nsRefPtr<MediaResource>

already_AddRefed<MediaResource>

::: content/media/plugins/MediaResourceServer.h
@@ +58,5 @@
> +  MediaResourceServer();
> +  NS_IMETHOD Run();
> +
> +public:
> +  // Create a MediaResourceServer and start it listening This call will

s/ listening This / listening. This / ?

@@ +67,5 @@
> +  void Stop();
> +
> +  // Add a MediaResource to be served by this server. Returns the
> +  // absolute URL that can be used to access the resource as a string.
> +  nsCString AddResource(mozilla::MediaResource* aResource);

I think you also need a way to remove the owning reference to the MediaResource held by the map.

	cajbir (:cajbir) Assignee
	Comment 4 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 769544 [details] [diff] [review]
> I think we should add MediaResource::ReadAt(char* buffer, uint32_t
> bufferLen, uint32_t* aOutBytesRead, int64_t offset), which allows us to
> seek+read in a safe way using the MediaCache's lock. We're having to code
> around this limitation in the WMF, OMX backends (and I had to in my mythical
> DirectShow backend too), so we may as well fix it properly here. Then you
> don't have to worry about concurrent reads and my comment about tell above;
> the read will be synchronized by the media cache.

I've spun this off into bug 894148.

	Peipei Cheng (needinfo if you need my action)
	Comment 5 • 11 years ago

doublec, has this been merged to 25?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 11 years ago

(In reply to pcheng from comment #5)
> doublec, has this been merged to 25?

pcheng - The patch is still being reviewed. It has not landed yet.

	cajbir (:cajbir) Assignee
	Comment 7 • 11 years ago

Addresses the review comments.

* Adds a way to remove entries from the map and does this when the decoder is destroyed.
* Uses already_AddRefed in places (I'd thought return value optimization would have dealt with the add/release optimization).
* Fixes ReadCRLF to deal with \r\n spanning buffers.
* Uses MediaResource::ReadAt to deal with seek/read thread issues.
* Early exits on failed writes.

	cajbir (:cajbir) Assignee
	Comment 8 • 11 years ago

This is an interdiff with changes between the original reviewed patch and the new patch containing review fixes.

	cajbir (:cajbir) Assignee
	Comment 9 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

Sotaro, cpearce asked me to ping your for your thoughts on this approach to  Android H.264 playback using stagefright where we create a mini-http server that the Android datasource connects to and that server proxies data from a MediaResource. 

In particular, can you think of any issues whereby Android would have threading issues with the approach.

	cajbir (:cajbir) Assignee
	Comment 10 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

cpearce has requested I get feedback from a netwerk peer to see if I'm using nsIServerSocket correctly in this patch. Would it be possible for you to take a look at MediaResourceServer.cpp and see if the code looks sane?

It creates a server socket that accepts connections from clients and acts as a mini-HTTP server. It is limited in protocol support to just that which Android Stagefright clients send and responds with data backed by a MediaResource.

	Sotaro Ikeda [:sotaro]
	Comment 11 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

The approach to use mini-http server is good. In the past, I saw similar implementation for DRM support in android. At that time, android did not have enough support for DRM. There should be no problem about threading in android. Only performance might be a problem.

I feel it might be better to have a comment about a way of using Decoder::mResource. At first it is used to store MediaResource and then used to store string.

	Sotaro Ikeda [:sotaro]
	Comment 12 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> The approach to use mini-http server is good. In the past, I saw similar
> implementation for DRM support in android. At that time, android did not
> have enough support for DRM.

I was a app's implementation.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 13 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

Looks good!

::: content/media/plugins/MediaResourceServer.cpp
@@ +66,5 @@
> +      else {
> +        eollast = false;
> +        aLine.Append('\r');
> +      }
> +    }           

nit: Trailing whitespace, and on line 87 too.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 14 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

Also, it would be good if you could add comments as to which methods of MediaResourceServer are threadsafe, or main thread only etc.

::: content/media/plugins/MediaResourceServer.cpp
@@ +182,5 @@
> +    // That is, the range request is of the form:
> +    //   Range: bytes=nnnn-
> +    // Were 'nnnn' is an integer number.
> +    NS_NAMED_LITERAL_CSTRING(byteRange, "Range: bytes=");
> +    char* s = strstr(line.get(), byteRange.get());

I got a compile error here on Windows:

 0:09.37 c:/Users/Bob/src/m-c/content/media/MediaResourceServer.cpp(186) : error C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
 0:09.37
 0:09.37         Conversion loses qualifiers

	Jason Duell
	Comment 15 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

This seems OK to me but I'd like Honza to look over it as he knows the nsIServerSocket API better than I do.

Honza, the only code that we need to look at here is MediaResourceServer.cpp|.h.

::: content/media/plugins/MediaResourceServer.cpp
@@ +332,5 @@
> +  rv = NS_NewThread(getter_AddRefs(thread));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  nsCOMPtr<nsIRunnable> event = new ServeResourceEvent(input.get(), output.get(), mServer);
> +  return thread->Dispatch(event, NS_DISPATCH_NORMAL);

So we dispatched the MediaResourceServer to the main thread here, but when OnSocketAccepted is called, we just wind up creating a new thread to run the ServeResourceEvent on.  Could we do without using the main thread at all here?   For instance, NS_NewThread winds up blocking the calling thread until the new thread is created and signals the main thread back, so if we could avoid doing that on the main thread it would be nice.

	Jason Duell
	Comment 16 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

:bsmith: this patch creates an internal HTTP proxy server on Android, only for serving up media resources (as a crash workaround--see comment 0).  I'm not sure if there are security/privacy issues here (other apps could potentially figure out the ephemeral port being used and sniff the media that's being served, but I can't figure out if that's a serious issue or not).  Opinion?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 17 • 11 years ago

I'm also investigating using this on Windows for interfacing with Windows Media Foundation and DirectShow, so we may want to use it on Windows too if it fixes some oranges.

	Honza Bambas (:mayhemer)
	Comment 18 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

I'd like to understand more what this is intended eventually to do.

As I understand (please fix me if I'm wrong):
- you have a host process and a plugin process
- the plugin process makes a request for a media resource
- currently (w/o this patch) we are using some android's API that is crashy
- so, you want to adjust/change the API plugins may use like this:
  - plugin process tells the host process to provide a media resource (that ends up at MediaPluginHost::CreateDecoder)
  - host process registers the resource and tells the plugin process to find it at e.g. http://127.0.0.1:8348/my-resource
  - plugin process connects the URL
  - host process parses the request, finds the previously registered resource
  - host process pushes the resource data to the http response
  - plugin process consumes the data
  - when done, all deregisters and closes

Is that so?

Is more work needed at DataSource::CreateFromURI?

::: content/media/plugins/MediaResourceServer.cpp
@@ +261,5 @@
> +    rv = mOutput->Write(b, len, &len);
> +    if (NS_FAILED (rv)) break;
> +
> +    rv =resource->ReadAt(start, b, 32768, &len);
> +  }

Not a good code... when the connection will be slow, you may load the resource data whole to the memory of this process.

You should use a stream copier or at least ReadSegments or WriteSegments depending on which of the two streams is buffered.  That will save memory coping and allocation a lot!

@@ +325,5 @@
> +  rv = aTrans->OpenInputStream(nsITransport::OPEN_BLOCKING, 0, 0, getter_AddRefs(input));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  rv = aTrans->OpenOutputStream(nsITransport::OPEN_BLOCKING, 0, 0, getter_AddRefs(output));
> +  if (NS_FAILED(rv)) return rv;

Hmm.. BLOCKING means also BUFFERED.  You will copy memory for all data unnecessarily at least 2 times.  One coping could be saved when you implement your server using only non-blocking streams.  That would make the code a little bit more complicated but more efficient.  You could then also use a thread pool with some reasonable thread limit (like 4 threads or so) to process asyncWait of async streams or target the stream copier and not spawn thread per request, it's wasting.

f- on these.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 19 • 11 years ago

(In reply to Jason Duell (:jduell) from comment #17)
> Comment on attachment 777570 [details] [diff] [review]
> Use DataSource::CreateFromURI instead of MediaStreamSource
> 
> Review of attachment 777570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :bsmith: this patch creates an internal HTTP proxy server on Android

I would also like to definitely use this on Windows for interfacing with DirectShow and Windows Media Foundation. I've got oranges in the WMF code that I've been trying for months to fix that are solved by using this proxy instead of implementing WMF's network stream abstraction as a wrapper around our MediaCache.

I tested on Try over the weekend, and my oranges disappeared.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 20 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

Also, it would be good if you could add comments as to which methods of MediaResourceServer are threadsafe, or main thread only etc.

::: content/media/plugins/MediaResourceServer.cpp
@@ +182,5 @@
> +    // That is, the range request is of the form:
> +    //   Range: bytes=nnnn-
> +    // Were 'nnnn' is an integer number.
> +    NS_NAMED_LITERAL_CSTRING(byteRange, "Range: bytes=");
> +    char* s = strstr(line.get(), byteRange.get());

I got a compile error here on Windows:

 0:09.37 c:/Users/Bob/src/m-c/content/media/MediaResourceServer.cpp(186) : error C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
 0:09.37
 0:09.37         Conversion loses qualifiers

@@ +220,5 @@
> +  nsAutoArrayPtr<char> b(new char[buffer_size]);
> +
> +  // If we know the length of the resource, send a Content-Length header.
> +  int64_t contentlength = resource->GetLength() - start;
> +  if (len > 0) {

Should this be "if (contentlength > 0)" ?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 21 • 11 years ago

Comment on attachment 777570 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

In general, the parser of the request at least needs to be more robust to handle the case where the request comes from something other than Android's media stack.

Also, what prevents another app on the phone, or even something from the outside, from connecting to this server and reading the resources from the media cache? It isn't clear to me from the patch.

::: content/media/plugins/MediaResourceServer.cpp
@@ +129,5 @@
> +{
> +  const char* url_start = strchr(aHTTPRequest.get(), ' ');
> +  if (!url_start) {
> +    return nullptr;
> +  }

You should verify that the request method is GET.

@@ +138,5 @@
> +  }
> +
> +  nsCString relative(url_start, url_end - url_start);
> +  nsRefPtr<MediaResource> resource =
> +    mServer->GetResource(mServer->GetURLPrefix() + relative);

You should verify that this is actually a relative path AND that it is one that doesn't start with ".." as many path traversal attacks do.

@@ +184,5 @@
> +    // Were 'nnnn' is an integer number.
> +    NS_NAMED_LITERAL_CSTRING(byteRange, "Range: bytes=");
> +    char* s = strstr(line.get(), byteRange.get());
> +    if (s) {
> +      start = strtoll(s+byteRange.Length(), NULL, 10);

IIRC, strtoll has a strange way of checking for errors. You are not checking for any errors here, but you should be.

Also, I suspect that it is likely that at some point in the future Android may request ranges that include an end. So, I recommend handling that here too.

@@ +205,5 @@
> +  if (start > 0 && !resource->IsTransportSeekable()) {
> +    start = 0;
> +  }
> +
> +  const char* response_line = start > 0 ?

note that start may be negative here.

@@ +219,5 @@
> +  const int buffer_size = 32768;
> +  nsAutoArrayPtr<char> b(new char[buffer_size]);
> +
> +  // If we know the length of the resource, send a Content-Length header.
> +  int64_t contentlength = resource->GetLength() - start;

You need to bounds-check start. Start could be negative and/or it could be greater than resource->GetLength().

@@ +220,5 @@
> +  nsAutoArrayPtr<char> b(new char[buffer_size]);
> +
> +  // If we know the length of the resource, send a Content-Length header.
> +  int64_t contentlength = resource->GetLength() - start;
> +  if (len > 0) {

It is definitely confusing why len is used sometimes and contentLength is used other times.

	cajbir (:cajbir) Assignee
	Comment 22 • 11 years ago

> I'd like to understand more what this is intended eventually to do.
> 
> As I understand (please fix me if I'm wrong):
> - you have a host process and a plugin process

Everything is done in the same process. There is no 'plugin' process. What happens is when a user requests a video on android we instantiate a socket server that acts as an HTTP proxy for the media file. This proxy forwards to a MediaResource which has a BLOCKing interface. Note that this is not a Gecko network stream - it's a Media specific class that exposes a blocking file interface.

Once this is created a Android libstagefright DataSource object is created that uses HTTP to retrieve the data to be decoded. It is pointed to our proxy so it will retrieve data from the MediaResource. This allows the android network routines to be used and makes handling things like DRM (in the future when we need to support it), and other Android specific things easier.

This is not expected to be a high performance server since it only retrieves on average one request at a time. For playback and for seeking.

> Is more work needed at DataSource::CreateFromURI?

I'm not sure what you are asking here.

> 
> ::: content/media/plugins/MediaResourceServer.cpp
> @@ +261,5 @@
> > +    rv = mOutput->Write(b, len, &len);
> > +    if (NS_FAILED (rv)) break;
> > +
> > +    rv =resource->ReadAt(start, b, 32768, &len);
> > +  }
> 
> Not a good code... when the connection will be slow, you may load the
> resource data whole to the memory of this process.

Can you expand on this? Which connection will be slow? Will the resource data be copied whole memory of process because mOutput is buffered (I thought socket output streams weren't buffered)? mOutput is a local socket so I can't see it being slow.

> You should use a stream copier or at least ReadSegments or WriteSegments
> depending on which of the two streams is buffered.  That will save memory
> coping and allocation a lot!

I can't use ReadSegments because 'resource' is not a stream, it's a MediaResource. I can't use WriteSegments because mOutput is a socket output stream and according to nsIOutputStream.idl:

>     * NOTE: this function may be unimplemented if a stream has no underlying
>     * buffer (e.g., socket output stream).

> Hmm.. BLOCKING means also BUFFERED.  You will copy memory for all data
> unnecessarily at least 2 times.  One coping could be saved when you
> implement your server using only non-blocking streams.  That would make the
> code a little bit more complicated but more efficient.  You could then also
> use a thread pool with some reasonable thread limit (like 4 threads or so)
> to process asyncWait of async streams or target the stream copier and not
> spawn thread per request, it's wasting.

Correct me if I'm wrong but I can't use async streams since MediaResource is blocking and we can't block during the async handling code. Can you let me know if this is possible?

Given that there will only be 1-2 connections on average and the connections to the server are all local to the device I'm not sure we need a thread pool.

	cajbir (:cajbir) Assignee
	Comment 23 • 11 years ago

(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #22)
> In general, the parser of the request at least needs to be more robust to
> handle the case where the request comes from something other than Android's
> media stack.

I think the parser is robust in that it gracefully fails for malformed requests, except for the checking of GET as you noted which I'll correct. If there are other areas where it is not please let me know.

> Also, what prevents another app on the phone, or even something from the
> outside, from connecting to this server and reading the resources from the
> media cache? It isn't clear to me from the patch.

The server is bound to the local interface so nothing from outside can connect to it. There is currently nothing stopping local programs from connecting however but they will need to:

1. Enumerate all the ports to find the port the server is bound to.
2. Guess the URL that the resource is being served on.

Number 2 is pretty easy given a malicious software author can read the source and it's just incrementing numbers. Would using a UUID or random number be sufficient for the URL address the security concern?

> You should verify that this is actually a relative path AND that it is one
> that doesn't start with ".." as many path traversal attacks do.

We are not using the path to access the filesystem. It is directly matched against an entry in a hash table and if found uses that to identify an existing in memory object used to retrieve data. Can you confirm that we don't need to do any ".." path traversal detecting in this case?

> Also, I suspect that it is likely that at some point in the future Android
> may request ranges that include an end. So, I recommend handling that here
> too.

We handle that in that we return a Content-Range header which basically states we ignored the end part of the range - it states we're returning the remaining data in the file. I can add specific 'end' handling though.

> It is definitely confusing why len is used sometimes and contentLength is
> used other times.

I'll change this and address your other review comments, thanks. Can you follow up with whether the path traversal comment and the changes for making it difficult for local connections from other apps are sufficient?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 24 • 11 years ago

(In reply to Chris Double (:doublec) from comment #24)
> (In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith)
> from comment #22)
> > In general, the parser of the request at least needs to be more robust to
> > handle the case where the request comes from something other than Android's
> > media stack.
> 
> I think the parser is robust in that it gracefully fails for malformed
> requests, except for the checking of GET as you noted which I'll correct. If
> there are other areas where it is not please let me know.

Just the things I noted in my feedback (e.g. out-of-range start values).

> The server is bound to the local interface so nothing from outside can
> connect to it. There is currently nothing stopping local programs from
> connecting however but they will need to:
> 
> 1. Enumerate all the ports to find the port the server is bound to.
> 2. Guess the URL that the resource is being served on.
> 
> Number 2 is pretty easy given a malicious software author can read the
> source and it's just incrementing numbers. Would using a UUID or random
> number be sufficient for the URL address the security concern?

A UUID isn't (necsesarily) unguessable. A 128-bit value generated from nsIRandomGenerator would be unguessable.

> We are not using the path to access the filesystem. It is directly matched
> against an entry in a hash table and if found uses that to identify an
> existing in memory object used to retrieve data. Can you confirm that we
> don't need to do any ".." path traversal detecting in this case?

I agree. Please add a comment to that effect in the code that parses the path.

> > Also, I suspect that it is likely that at some point in the future Android
> > may request ranges that include an end. So, I recommend handling that here
> > too.
> 
> We handle that in that we return a Content-Range header which basically
> states we ignored the end part of the range - it states we're returning the
> remaining data in the file. I can add specific 'end' handling though.

It is up to you whichever is better. But, it is a good idea to, at least, document what was decided in the code, so that it is clear that it wasn't an oversight.

I guess it might be possible for another application to somehow detect what path was requested after a request has been made. If we expect that these requests are supposed to be made only once, it would be best to enforce that (i.e. remove the entry from the internal table) as part of servicing the request.

I am not sure how the media cache deals with no-cache/no-store/etc. Is it worth forwarding those on to Android client?

On Windows, I would not be surprised to find anti-malware/firewall applications blocking this from working and/or warning the user that bad things are happening.

Generally, I would prefer the finding of a way to solve these problems without us implementing an HTTP server within Firefox. Exposing a whole new IPC interface to all other programs on the system is excessive in the attack surface that it adds, considering that really the data being exposed by the server only needs to be accessed by the OS. OTOH, I don't know enough about the problem space to offer an alternative; also, local attacks via IPC from other software running on the system are far from Firefox's biggest security problem. I recommend chatting with people from security assurance to see what things I've overlooked.

	Peipei Cheng (needinfo if you need my action)
	Comment 25 • 11 years ago

Any reason that we remove the tracking-firefox23 flags? Considering this bug blocks bug 894893 which is targeted for Fennec V23, is it possible to include this fix in V23?

	Scoobidiver (away)
	Comment 26 • 11 years ago

(In reply to pcheng from comment #26)
> Any reason that we remove the tracking-firefox23 flags?
It shouldn't have been. It's an exclusive release drivers' permission.

> Considering this bug blocks bug 894893 which is targeted for Fennec V23, is it possible
> to include this fix in V23?
23.0 RC is built (see http://hg.mozilla.org/releases/mozilla-beta/graph) so any bugs tracked for 23.0 will be marked as wontfix for this version. In addition, this patch is not ready yet and would have been too risky to uplift so late in the cycle.

	cajbir (:cajbir) Assignee
	Comment 27 • 11 years ago

(In reply to Scoobidiver from comment #27)
> (In reply to pcheng from comment #26)
> > Any reason that we remove the tracking-firefox23 flags?
> It shouldn't have been. It's an exclusive release drivers' permission.

All the flags got reset when I added a comment for some reason. I don't know why this happened since I don't have permission to update the flags.

	bhavana bajaj [:bajaj]
	Comment 28 • 11 years ago

Setting back the tracking flags per comment# 23, which accidentally changed the flags.

	cajbir (:cajbir) Assignee
	Comment 29 • 11 years ago

Addresses review comments. I'll attach an interdiff.

	cajbir (:cajbir) Assignee
	Comment 30 • 11 years ago

Interdiff for recent patch. Brian, this includes the changes you suggested.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 31 • 11 years ago

Comment on attachment 785591 [details] [diff] [review]
interdiff.patch

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

::: content/media/plugins/MediaResourceServer.cpp
@@ +306,3 @@
>      if (NS_FAILED (rv)) break;
>  
> +    rv =resource->ReadAt(start, b, 32768, &bytesRead);

Nit:
rv = resource->ReadAt(start, b, 32768, &bytesRead);
(insert space after "rv =")

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 32 • 11 years ago

Comment on attachment 785590 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

LGTM.

	Honza Bambas (:mayhemer)
	Comment 33 • 11 years ago

(In reply to Chris Double (:doublec) from comment #23)
> > I'd like to understand more what this is intended eventually to do.
> > 
> > As I understand (please fix me if I'm wrong):
> > - you have a host process and a plugin process
> 
> Everything is done in the same process. There is no 'plugin' process. What
> happens is when a user requests a video on android we instantiate a socket
> server that acts as an HTTP proxy for the media file. This proxy forwards to
> a MediaResource which has a BLOCKing interface. Note that this is not a
> Gecko network stream - it's a Media specific class that exposes a blocking
> file interface.

I'm taking about transport streams you open @ OnSocketAccepted.  Those are necko streams.

What I wanted to point out was that you should prevent unnecessary memory copying which causes memory consumption and CPU load.

> 
> Once this is created a Android libstagefright DataSource object is created
> that uses HTTP to retrieve the data to be decoded. It is pointed to our
> proxy so it will retrieve data from the MediaResource. This allows the
> android network routines to be used and makes handling things like DRM (in
> the future when we need to support it), and other Android specific things
> easier.
> 
> This is not expected to be a high performance server since it only retrieves
> on average one request at a time. For playback and for seeking.

If all the media content will go though it then it MUST be well optimized.  Looking at the code as is tells me there may be more work to do.

> 
> > Is more work needed at DataSource::CreateFromURI?
> 
> I'm not sure what you are asking here.

Moot after your explanation.

> 
> > 
> > ::: content/media/plugins/MediaResourceServer.cpp
> > @@ +261,5 @@
> > > +    rv = mOutput->Write(b, len, &len);
> > > +    if (NS_FAILED (rv)) break;
> > > +
> > > +    rv =resource->ReadAt(start, b, 32768, &len);
> > > +  }
> > 
> > Not a good code... when the connection will be slow, you may load the
> > resource data whole to the memory of this process.
> 
> Can you expand on this? Which connection will be slow? 

The loopback connection obviously.

> Will the resource
> data be copied whole memory of process because mOutput is buffered (I
> thought socket output streams weren't buffered)? 

You open them such way (as buffered).

> mOutput is a local socket
> so I can't see it being slow.

When system is busy it could be.

> 
> > You should use a stream copier or at least ReadSegments or WriteSegments
> > depending on which of the two streams is buffered.  That will save memory
> > coping and allocation a lot!
> 
> I can't use ReadSegments because 'resource' is not a stream, it's a
> MediaResource. I can't use WriteSegments because mOutput is a socket output
> stream and according to nsIOutputStream.idl:
> 
> >     * NOTE: this function may be unimplemented if a stream has no underlying
> >     * buffer (e.g., socket output stream).

But the way you open the streams of socket transport you do open them as buffered.  OPEN_BLOCKING implies OPEN_BUFFERED.  See the code at http://hg.mozilla.org/mozilla-central/annotate/dbd7d55d64ff/netwerk/base/src/nsSocketTransport2.cpp#l1758

> 
> > Hmm.. BLOCKING means also BUFFERED.  You will copy memory for all data
> > unnecessarily at least 2 times.  One coping could be saved when you
> > implement your server using only non-blocking streams.  That would make the
> > code a little bit more complicated but more efficient.  You could then also
> > use a thread pool with some reasonable thread limit (like 4 threads or so)
> > to process asyncWait of async streams or target the stream copier and not
> > spawn thread per request, it's wasting.
> 
> Correct me if I'm wrong but I can't use async streams since MediaResource is
> blocking and we can't block during the async handling code. Can you let me
> know if this is possible?

Hmm..  You may implement nsIInputStream that wraps the MediaResource.  This way you can use stream copier (via WriteSegments to the socket output stream).   Then you have to have a separate thread for each stream copier instance to not block one copying process by another.

> 
> Given that there will only be 1-2 connections on average and the connections
> to the server are all local to the device I'm not sure we need a thread pool.

Depends.  Maybe for the start it will be OK w/o a pool.

BTW, this all can be done in a followup.  If you urgently needs this patch to fix a crash, then go on.  However, since there will be large amount of data flow, we should do it right.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 34 • 11 years ago

I am on PTO starting now so, if you need this reviewed before the 19th, please ask another Necko peer to review the input sanitation code. Also, it sounds like a good idea to have somebody from security assurance look at it too.

	cajbir (:cajbir) Assignee
	Comment 35 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #34)
> BTW, this all can be done in a followup.  If you urgently needs this patch
> to fix a crash, then go on.  However, since there will be large amount of
> data flow, we should do it right.

Thanks for the comments. I've raised bug Bug 906929 as a followup and will address post landing if someone doesn't get to it first.

	cajbir (:cajbir) Assignee
	Comment 36 • 11 years ago

I'd like to get security review on the design of this patch. See the start of comment 23 for some details. The patch implements an internal socket server that receives HTTP requests from an operating system library (libstagefright) what we don't have source for. The internal socket server acts as an HTTP proxy to a MediaResource (content/media/MediaResource.{h,cpp}). The URLs on the HTTP proxy are randomly generated. The socket server is bound to localhost. The code for the socket server is in content/media/plugins/MediaResourceServer.{h,cpp}.

	Daniel Veditz [:dveditz]
	Comment 37 • 11 years ago

Comment on attachment 785590 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

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

::: content/media/plugins/MediaResourceServer.cpp
@@ +18,5 @@
> +#include "MediaResourceServer.h"
> +
> +#if defined(_MSC_VER)
> +#define strtoll _strtoi64
> +#define snprintf _snprintf

Please define this to _snprintf_s -- the MSVC version of _snprintf will leave overlong strings unterminated (original 1980s behavior?). The _s version always null terminates even on truncation as expected everywhere else.

@@ +479,5 @@
> +  if (NS_FAILED (rv)) return rv;
> +
> +  {
> +    MutexAutoLock lock(mMutex);
> +    mResources[url] = aResource;

The odds on a collision are astronomically low but if it happens we get at least a MediaResource leak. What would happen when one consumer keeps using the resource after the first guy finishes and calls RemoveResource()?

	Daniel Veditz [:dveditz]
	Comment 38 • 11 years ago

This approach looks safe with the addition of random URLs in the most recent patch.

	cajbir (:cajbir) Assignee
	Comment 39 • 11 years ago

Review comments addressed.

(In reply to Daniel Veditz [:dveditz] from comment #38)
> The odds on a collision are astronomically low but if it happens we get at
> least a MediaResource leak. What would happen when one consumer keeps using
> the resource after the first guy finishes and calls RemoveResource()?

I've changed this to make it an error if an attempt is made to add a resource URL that already exists. I've also made the _snprintf_s change you requested.

r+ from cpearce carried forward.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 40 • 11 years ago

Comment on attachment 785591 [details] [diff] [review]
interdiff.patch

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

I don't see any defects in the code.

I think it is better in general for security if we try to avoid The C str* functions and raw buffer handling when we can. Code that uses ns[AC]String and/or PR_smprintf and friends is less likely to have tragic flaws. I think (#define snprintf _snprintf) is a particularly dangerous thing to do in general given the semantic differences between snprintf _snprintf, though in these specific uses you seem to be doing something that will work correctly.

	cajbir (:cajbir) Assignee
	Comment 41 • 11 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=b10098d192f6

	cajbir (:cajbir) Assignee
	Comment 42 • 11 years ago

Pre-landing testing finds last minute bug in clamping of seeking values introduced during review fix. This patch has fix (min/max were swamped when clamping).

	cajbir (:cajbir) Assignee
	Comment 43 • 11 years ago

Oops, wrong patch. Correct one attached.

	cajbir (:cajbir) Assignee
	Comment 44 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d20975bbdb9

	Ed Morley [:emorley]
	Comment 45 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2d20975bbdb9

	Alex Keybl [:akeybl]
	Comment 46 • 11 years ago

Likely too late to take a fix in the FF24 timeframe, but how do you feel about uplifting to Aurora 25 Chris?

	Alex Keybl [:akeybl]
	Comment 47 • 11 years ago

(In reply to Alex Keybl [:akeybl] from comment #47)
> Likely too late to take a fix in the FF24 timeframe, but how do you feel
> about uplifting to Aurora 25 Chris?

My mistake, I didn't recognize this bug when triaging. This is still important to a 23.0.2 and 24.

	cajbir (:cajbir) Assignee
	Comment 48 • 11 years ago

Comment on attachment 797654 [details] [diff] [review]
Use DataSource::CreateFromURI instead of MediaStreamSource

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Stagefright video decoding crashes
User impact if declined: This patch is required for a 23.0.2 release to fix a crash with a particular device. Without the patch that device can't play video in firefox. We want to move to Aurora to get more testing.
Testing completed (on m-c, etc.): It has had minimal testing and has been on m-c for about a week. The intent is for Aurura to enable a wider base of testing.
Risk to taking this patch (and alternatives if risky): HTML video playback could stop working on existing devices.
String or IDL/UUID changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/e484bfe5427e