Introduce a client minimum picture pool size

ppapi/proxy/video_decoder_resource.cc

Index: ppapi/proxy/video_decoder_resource.cc
diff --git a/ppapi/proxy/video_decoder_resource.cc b/ppapi/proxy/video_decoder_resource.cc
index bcc0e6e844a8428674bfb6076a1ddcaad397009e..ed4dae0572d41a98b2327038d58aea1024e6513e 100644
--- a/ppapi/proxy/video_decoder_resource.cc
+++ b/ppapi/proxy/video_decoder_resource.cc
@@ -29,6 +29,15 @@ using ppapi::thunk::PPB_VideoDecoder_API;
 namespace ppapi {
 namespace proxy {
 
+namespace {
+
+// The maximum number of pictures that the client can pass in for
+// min_picture_count, just as a sanity check on the argument.
+// This should match the constant of the same name in test_video_decoder.cc.
+const uint32_t kMaximumPictureCount = 100;

[jln (very slow on Chromium), 2015/08/17 21:44:39]

What happens if they don't match?

Could you expose a GetMaximumPictureCount() so that you can test that they match
at least?

[lpique, 2015/08/17 23:57:07]

The test in test_video_decoder.cc will fail, since it tries to use a count of
100, expecting it to pass, and a count of 101 expecting it to fail.
This question gave me the most pause.

Yes I could use this function to elimiante the constant from the test (it would
just use the returned value, and try calling Initialize() with the returned
count and one more than the returned count).

But it is not really useful otherwise. The limit of 100 I chose is purely
arbitrary. For the purposes of my work on ARC (a client of this API) I only
actually need a minimum picture count of 8. I chose a larger  value in case that
changes, but 100 is pretty extreme.

For video decoding, a normal client will want to minimize this value. Large
values represent more memory being allocated, as well as more latency (ARC, or
really the Android code it uses has additional logic to sync things up despite
having a larger decode queue)

A normal client would have some simple known fixed constant need, and I think it
is perfectly fine if it tries to call Initialize() with this value, and just
asserts that it returns PP_OK. Being able to call GetMaximumPictureCount() might
be useful for displaying an error message, but its not really useful otherwise.
If the limit is too small, the only way to increase it is to rebuild Chrome with
a larger value.

+
+}  // namespace
+
 VideoDecoderResource::ShmBuffer::ShmBuffer(
     scoped_ptr<base::SharedMemory> shm_ptr,
     uint32_t size,
@@ -62,6 +71,7 @@ VideoDecoderResource::VideoDecoderResource(Connection connection,
                                            PP_Instance instance)
     : PluginResource(connection, instance),
       num_decodes_(0),
+      min_picture_count_(0),
       get_picture_(NULL),
       get_picture_0_1_(NULL),
       gles2_impl_(NULL),
@@ -98,6 +108,19 @@ int32_t VideoDecoderResource::Initialize0_1(
                     allow_software_fallback
                         ? PP_HARDWAREACCELERATION_WITHFALLBACK
                         : PP_HARDWAREACCELERATION_ONLY,
+                    0,
+                    callback);
+}
+
+int32_t VideoDecoderResource::Initialize0_2(
+    PP_Resource graphics_context,
+    PP_VideoProfile profile,
+    PP_HardwareAcceleration acceleration,
+    scoped_refptr<TrackedCallback> callback) {
+  return Initialize(graphics_context,
+                    profile,
+                    acceleration,
+                    0,
                     callback);
 }
 
@@ -105,16 +128,21 @@ int32_t VideoDecoderResource::Initialize(
     PP_Resource graphics_context,
     PP_VideoProfile profile,
     PP_HardwareAcceleration acceleration,
+    uint32_t min_picture_count,
     scoped_refptr<TrackedCallback> callback) {
   if (initialized_)
     return PP_ERROR_FAILED;
   if (profile < 0 || profile > PP_VIDEOPROFILE_MAX)
     return PP_ERROR_BADARGUMENT;
+  if (min_picture_count > kMaximumPictureCount)

[bbudge, 2015/08/19 18:47:05]

You could keep this if you move the constant definition. See my comment on the
host.

+    return PP_ERROR_BADARGUMENT;
   if (initialize_callback_.get())
     return PP_ERROR_INPROGRESS;
   if (!graphics_context)
     return PP_ERROR_BADRESOURCE;
 
+  min_picture_count_ = min_picture_count;
+
   HostResource host_resource;
   if (!testing_) {
     // Create a new Graphics3D resource that can create texture resources to
@@ -144,7 +172,7 @@ int32_t VideoDecoderResource::Initialize(
   Call<PpapiPluginMsg_VideoDecoder_InitializeReply>(
       RENDERER,
       PpapiHostMsg_VideoDecoder_Initialize(
-          host_resource, profile, acceleration),
+          host_resource, profile, acceleration, min_picture_count),
       base::Bind(&VideoDecoderResource::OnPluginMsgInitializeComplete, this));
 
   return PP_OK_COMPLETIONPENDING;
@@ -359,6 +387,7 @@ void VideoDecoderResource::OnPluginMsgRequestTextures(
     uint32_t texture_target,
     const std::vector<gpu::Mailbox>& mailboxes) {
   DCHECK(num_textures);
+  DCHECK(num_textures >= min_picture_count_);

[jln (very slow on Chromium), 2015/08/17 21:44:39]

Is this correct as a DCHECK? What happens if an attacker sends a malicious
min_picture_count?

[lpique, 2015/08/17 23:57:07]

I think so. With the rest of my changes, this should never be trigger, as later
code ensures that num_textures >= min_picture_count_.

I left this here in case some introduces a new code path that does not respect
the minimum requested, in which case the test for allocating the maximum allowed
minimum picture count should trigger the DCHECK (unless I am mistaken about the
DCHECK's being enabled for the test, which is possible)

If this DCHECK is compiled out, and new code is written in the host that does
not honor the requested minimum picture count (num_textures <
min_picture_count), the worst that happens is that the client may try to
allocate a larger number of textures than the interface can provide, and the
client ends up waiting on the callback from GetPicture() to complete.
The check I added at line 137 is supposed to restrict the set of values to a
reasonable range.

Without that check, the client can request say MAXINT, causing the host to try
to allocate memory for MAXINT picture frames. That should fail due to running
out of memory, but I certainly didn't expect the host code to handle it
correctly.

However looking back on it now, I'm fairly certain that I need to move the
validation to pepper_video_decoder_host.cc, as I think the client could bypass
the check here. (I saw the other validation here, and so I added it here.)

[bbudge, 2015/08/18 00:48:13]

You definitely have to validate anything you receive from the plugin in the
renderer process. Any validation done in the resource (plugin process) is purely
for convenience and can easily be bypassed by an attacker. Good catch jln.

[bbudge, 2015/08/19 18:47:06]

The DCHECKs here are OK for validating proxy correctness. They aren't a security
check, since this is a message from a more trusted (renderer) process.
'min_picture_count_' is completely under the control of the untrusted process.

   DCHECK(mailboxes.empty() || mailboxes.size() == num_textures);
   std::vector<uint32_t> texture_ids(num_textures);
   if (gles2_impl_) {