Issue 1092005

src/trusted/plugin/srpc/srpc.cc

Index: src/trusted/plugin/srpc/srpc.cc
===================================================================
--- src/trusted/plugin/srpc/srpc.cc	(revision 1695)
+++ src/trusted/plugin/srpc/srpc.cc	(working copy)
@@ -355,7 +355,7 @@
   if (NULL == stream->notifyData) {
     // Closures are handled in URLNotify (this is the only way to know
     // the stream is completed since not all streams have a valid "end" value
-    // Here we handle only the default, src=...  streams (statically obtained)
+    // Here we handle only the default, nexe=...  streams (statically obtained)
     if (static_cast<int32_t>(stream->end) == offset + len) {
       // Stream downloaded - go ahead
       dprintf(("default run\n"));
@@ -389,7 +389,7 @@
   if (NULL != closure) {
     closure->Run(stream, fname);
   } else {
-    // default, src=... statically obtained
+    // default, nexe=... statically obtained
     dprintf(("default run\n"));
     nacl_srpc::Plugin* real_plugin =
         static_cast<nacl_srpc::Plugin*>(plugin_->get_handle());
@@ -415,6 +415,10 @@
   nacl_srpc::Closure* closure
       = static_cast<nacl_srpc::Closure*>(notifyData);
 
+  // FIXME(adonovan): this looks fishy!  closure is maybe-null in the
+  // false branch, but assumed non-null in the true branch... yet the
+  // branch condition is not under our control.

[sehr (please use chromium), 2010/04/07 03:57:39]

I fear this code has changed somewhat due Bennet's recent change.  I don't think
it's substantially different, though.

NPP_URLNotify is invoked in (to the best of my knowledge) exactly three
situations:
1) When a nacl module load is requested due to src= being changed.
2) When a resource is requested by either __UrlAsNaClDesc or NPN_GetURL{Notify}.
3) When a nacl module load happens implicitly because src= was set in the embed
tag or data= in the object tag.

(1) and (2) have non-null closures explicitly.
(3) is potentially at issue.  I like your change because it fuses (3) into (1).

+
   if (NPRES_DONE == reason) {
     dprintf(("URLNotify: '%s', rsn NPRES_DONE (%d)\n", url, reason));
     StreamBuffer* stream_buffer = closure->buffer();
@@ -431,6 +435,11 @@
     if (NULL != closure) {
       closure->Run(NULL, NULL);
     } else {
+      // FIXME(adonovan): I don't understand how this can work.
+      // RunOnfailHandler's docstring says "to indicate unsuccessful
+      // loading of a module", but if we're loading a module, then
+      // |closure| is an instance of LoadNaClAppNotify, so it's
+      // non-NULL, and the other branch of the if-statement is taken.

[bsy, 2010/03/23 21:20:37]

plz verify w/ sehr, but i think this distinction is whether the load was
initiated due to the <EMBED> tag or via NPN_URLNotify, where the plugin is
asking the browser to load something.  in the latter case, there is no on
failure handler, since the controlling code is C/C++, not javascript.

[sehr (please use chromium), 2010/03/23 21:37:16]

Bennet got this right.  We use the same code to download other assets (in the
SRPC-only world) that we use to download module.

[adonovan, 2010/03/25 00:44:05]

Regarding the onfail handler, I can't actually find any respectable
specifications of this HTML feature, so I guess it can't be that important.  In
any case, this is rather subtle and should probably be documented.


But regarding the data flow in this function, I still think it is unsound. 
Consider this: you cannot predict "reason"; it is determined by events outside
the control of the plugin, so its value is arbitrary, and not correlated with
the predicate "closure != NULL".  So, you don't learn the value of this
predicate based on the conditional control flow.  Yet the two arms of the
conditional make incompatible assumptions, the first that it is always true, the
second that it may be false.

If the first is right, then the onfail handler is unreachable.  If the second is
right, then we're possibly dereferencing a null pointer.

[sehr (please use chromium), 2010/04/07 03:57:39]

I think this block is because of (3) above.  But you're right that either up
there or down here has a problem.

       plugin()->get_handle()->GetPortablePluginInterface()->RunOnfailHandler(
           GetPluginIdentifier());
     }