Added an extra sync token field for extra command buffer identification.

gpu/ipc/gpu_command_buffer_traits.cc

Index: gpu/ipc/gpu_command_buffer_traits.cc
diff --git a/gpu/ipc/gpu_command_buffer_traits.cc b/gpu/ipc/gpu_command_buffer_traits.cc
index a1932eede1b2da38e83a031047c77c801409d6ff..72a342413c72cf410798dcfd8e30660379651d77 100644
--- a/gpu/ipc/gpu_command_buffer_traits.cc
+++ b/gpu/ipc/gpu_command_buffer_traits.cc
@@ -59,6 +59,7 @@ void ParamTraits<gpu::SyncToken>::Write(Message* m, const param_type& p) {
 
   WriteParam(m, p.verified_flush());
   WriteParam(m, p.namespace_id());
+  WriteParam(m, p.extra_data_field());
   WriteParam(m, p.command_buffer_id());
   WriteParam(m, p.release_count());
 }
@@ -69,17 +70,19 @@ bool ParamTraits<gpu::SyncToken>::Read(const Message* m,
   bool verified_flush = false;
   gpu::CommandBufferNamespace namespace_id =
       gpu::CommandBufferNamespace::INVALID;
+  uint32_t extra_data_field = 0;
   uint64_t command_buffer_id = 0;
   uint64_t release_count = 0;
 
   if (!ReadParam(m, iter, &verified_flush) ||
       !ReadParam(m, iter, &namespace_id) ||
+      !ReadParam(m, iter, &extra_data_field) ||

[piman, 2015/11/30 20:59:23]

Actually, I have a security concern: the rest of the code will assume that the
stream id stored in this field is the one that corresponds to command_buffer_id,
but we have no way to validate it here. I'm worried that we'd make
security-sensitive decisions on it when an untrusted client could craft a
SyncToken where they don't match.

My preference would actually be to not send this over IPC, and handle the
possibility that it's 0 (which is invalid) in e.g. CanWaitUnverifiedSyncToken
(if stream_id == 0, then no, we can't wait on that token).

The argument is that it's only used on the client to stash the stream_id for
same-process synchronization, but as soon as we're talking cross-process
synchronization, the stream_id shouldn't be relevant anymore.

[David Yen, 2015/11/30 22:45:06]

Since I have created this variable as a "generic store what you need into sync
token" I'm worried someone might come up with a use later in mojo or something
which they expect it to make it across process. This variable only being
available within a single process is a huge gotcha.

From the point of view of security, you're right that we do always have to
account for invalid data. I think all the data in a sync token should always be
viewed as "untrusted" though, and we already have to account for that invalid
data for all the other values. Having said that, you reminded me that the
FlushPendingStream needs to account for having an invalid stream id. I will
change the FlushPendingStream() to use find instead of operator [].

       !ReadParam(m, iter, &command_buffer_id) ||
       !ReadParam(m, iter, &release_count)) {
     return false;
   }
 
-  p->Set(namespace_id, command_buffer_id, release_count);
+  p->Set(namespace_id, extra_data_field, command_buffer_id, release_count);
   if (p->HasData()) {
     if (!verified_flush)
       return false;
@@ -91,8 +94,10 @@ bool ParamTraits<gpu::SyncToken>::Read(const Message* m,
 
 void ParamTraits<gpu::SyncToken>::Log(const param_type& p, std::string* l) {
   *l +=
-      base::StringPrintf("[%d:%llX] %llu", static_cast<int>(p.namespace_id()),
+      base::StringPrintf("[%d:%llX:%u] %llu",
+                         static_cast<int>(p.namespace_id()),
                          static_cast<unsigned long long>(p.command_buffer_id()),
+                         p.extra_data_field(),
                          static_cast<unsigned long long>(p.release_count()));
 }