Mac: Ensure that Mach messages always get a reply

content/browser/browser_io_surface_manager_mac.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/browser_io_surface_manager_mac.h"
 
 #include <servers/bootstrap.h>
 
 #include <string>
 
@@ skipping 181 matching lines @@
   }
 
   union {
     mach_msg_header_t header;
     IOSurfaceManagerMsg_RegisterIOSurfaceReply register_io_surface;
     IOSurfaceManagerMsg_AcquireIOSurfaceReply acquire_io_surface;
   } reply = {{0}};
 
   switch (request.msg.header.msgh_id) {
     case IOSurfaceManagerHostMsg_RegisterIOSurface::ID:
-      if (!HandleRegisterIOSurfaceRequest(request.msg.register_io_surface,
-                                          &reply.register_io_surface)) {
-        return;
-      }
+      HandleRegisterIOSurfaceRequest(request.msg.register_io_surface,
+                                     &reply.register_io_surface);
       break;
     case IOSurfaceManagerHostMsg_UnregisterIOSurface::ID:
       HandleUnregisterIOSurfaceRequest(request.msg.unregister_io_surface);
       // Unregister requests are asynchronous and do not require a reply as
       // there is no guarantee for how quickly an IO surface is removed from
       // the IOSurfaceManager instance after it has been deleted by a child
       // process.
       return;
     case IOSurfaceManagerHostMsg_AcquireIOSurface::ID:
-      if (!HandleAcquireIOSurfaceRequest(request.msg.acquire_io_surface,
-                                         &reply.acquire_io_surface)) {
-        return;
-      }
+      HandleAcquireIOSurfaceRequest(request.msg.acquire_io_surface,
+                                    &reply.acquire_io_surface);
       break;
     default:
       LOG(ERROR) << "Unknown message received!";
       return;
   }
 
   kr = mach_msg(&reply.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT,
                 reply.header.msgh_size, 0, MACH_PORT_NULL, kSendReplyTimeoutMs,
                 MACH_PORT_NULL);
   if (kr != KERN_SUCCESS) {
     MACH_LOG(ERROR, kr) << "mach_msg";
   }
 }
 
-bool BrowserIOSurfaceManager::HandleRegisterIOSurfaceRequest(
+void BrowserIOSurfaceManager::HandleRegisterIOSurfaceRequest(
     const IOSurfaceManagerHostMsg_RegisterIOSurface& request,
     IOSurfaceManagerMsg_RegisterIOSurfaceReply* reply) {
   base::AutoLock lock(lock_);
 
+  reply->header.msgh_bits = MACH_MSGH_BITS_REMOTE(request.header.msgh_bits);
+  reply->header.msgh_remote_port = request.header.msgh_remote_port;
+  reply->header.msgh_size = sizeof(*reply);
+  reply->result = false;
+
   IOSurfaceManagerToken token;
   static_assert(sizeof(request.token_name) == sizeof(token.name),
                 "Mach message token size doesn't match expectation.");
   token.SetName(request.token_name);
   if (token.IsZero() || token != gpu_process_token_) {
     LOG(ERROR) << "Illegal message from non-GPU process!";
-    return false;
+    return;
   }
 
   IOSurfaceMapKey key(IOSurfaceId(request.io_surface_id), request.client_id);
   io_surfaces_.add(key, make_scoped_ptr(new base::mac::ScopedMachSendRight(
                             request.io_surface_port.name)));
 
-  reply->header.msgh_bits = MACH_MSGH_BITS_REMOTE(request.header.msgh_bits);
-  reply->header.msgh_remote_port = request.header.msgh_remote_port;
-  reply->header.msgh_size = sizeof(*reply);
   reply->result = true;
-  return true;
 }
 
 bool BrowserIOSurfaceManager::HandleUnregisterIOSurfaceRequest(
     const IOSurfaceManagerHostMsg_UnregisterIOSurface& request) {
   base::AutoLock lock(lock_);
 
   IOSurfaceManagerToken token;
   static_assert(sizeof(request.token_name) == sizeof(token.name),
                 "Mach message token size doesn't match expectation.");
   token.SetName(request.token_name);
   if (token.IsZero() || token != gpu_process_token_) {
     LOG(ERROR) << "Illegal message from non-GPU process!";
     return false;
   }
 
   IOSurfaceMapKey key(IOSurfaceId(request.io_surface_id), request.client_id);
   io_surfaces_.erase(key);
   return true;
 }
 
-bool BrowserIOSurfaceManager::HandleAcquireIOSurfaceRequest(
+void BrowserIOSurfaceManager::HandleAcquireIOSurfaceRequest(
     const IOSurfaceManagerHostMsg_AcquireIOSurface& request,
     IOSurfaceManagerMsg_AcquireIOSurfaceReply* reply) {
   base::AutoLock lock(lock_);
 
+  reply->header.msgh_bits =
+      MACH_MSGH_BITS_REMOTE(request.header.msgh_bits) | MACH_MSGH_BITS_COMPLEX;
+  reply->header.msgh_remote_port = request.header.msgh_remote_port;
+  reply->header.msgh_size = sizeof(*reply);
+  reply->result = false;

[reveman, 2015/09/18 18:22:30]

Request: This is not new to this patch but can you change "reply->result =
false;" to the following instead of depending on the caller initializing |reply|
to 0:

reply->body.msgh_descriptor_count = 0;
reply->result = false;
reply->io_surface_port.name = MACH_PORT_NULL;
reply->io_surface_port.disposition = 0;
reply->io_surface_port.type = 0;

[ccameron, 2015/09/18 18:36:00]

Done. Also changed Register reply to set body.* to 0 as well.

[reveman, 2015/09/18 18:47:48]

Ah, I guess we should really be doing a bzero(reply, sizeof(*reply)) to not
depend on reply already being 0 initialized... current patch is fine though

+
   IOSurfaceManagerToken token;
   static_assert(sizeof(request.token_name) == sizeof(token.name),
                 "Mach message token size doesn't match expectation.");
   token.SetName(request.token_name);
   auto child_process_id_it = child_process_ids_.find(token);
   if (child_process_id_it == child_process_ids_.end()) {
     LOG(ERROR) << "Illegal message from non-child process!";
-    return false;
+    return;
   }
 
-  reply->header.msgh_bits =
-      MACH_MSGH_BITS_REMOTE(request.header.msgh_bits) | MACH_MSGH_BITS_COMPLEX;
-  reply->header.msgh_remote_port = request.header.msgh_remote_port;
-  reply->header.msgh_size = sizeof(*reply);
-
+  reply->result = true;
   IOSurfaceMapKey key(IOSurfaceId(request.io_surface_id),
                       child_process_id_it->second);
   auto it = io_surfaces_.find(key);
   if (it == io_surfaces_.end()) {
     LOG(ERROR) << "Invalid Id for IOSurface " << request.io_surface_id;
-    return true;
+    return;
   }
 
   reply->body.msgh_descriptor_count = 1;
   reply->io_surface_port.name = it->second->get();
   reply->io_surface_port.disposition = MACH_MSG_TYPE_COPY_SEND;
   reply->io_surface_port.type = MACH_MSG_PORT_DESCRIPTOR;
-  return true;
 }
 
 }  // namespace content