Pickle: handle invalid data on 64 bit systems....

base/pickle.cc

 // Copyright (c) 2006-2008 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 "base/pickle.h"
 
 #include <stdlib.h>
 
 #include <algorithm>  // for max()
 #include <limits>
@@ skipping 23 matching lines @@
       capacity_(0),
       variable_buffer_offset_(0) {
   DCHECK(static_cast<size_t>(header_size) >= sizeof(Header));
   DCHECK(header_size <= kPayloadUnit);
   Resize(kPayloadUnit);
   header_->payload_size = 0;
 }
 
 Pickle::Pickle(const char* data, int data_len)
     : header_(reinterpret_cast<Header*>(const_cast<char*>(data))),
-      header_size_(data_len - header_->payload_size),
+      header_size_(0),
       capacity_(kCapacityReadOnly),
       variable_buffer_offset_(0) {
-  DCHECK(header_size_ >= sizeof(Header));
-  DCHECK(header_size_ == AlignInt(header_size_, sizeof(uint32)));
+  if (data_len >= static_cast<int>(sizeof(Header)))
+    header_size_ = data_len - header_->payload_size;
+
+  if (header_size_ > static_cast<unsigned int>(data_len))
+    header_size_ = 0;
+
+  if (header_size_ != AlignInt(header_size_, sizeof(uint32)))
+    header_size_ = 0;
+
+  // If there is anything wrong with the data, we're not going to use it.
+  if (!header_size_)
+    header_ = NULL;

[darin (slow to review), 2010/11/11 17:33:57]

I'm a little concerned about header_ being null.  Previously, we always
guaranteed that it would not be null.  Maybe we should CHECK(false) instead?

Can you tell me more about why you want this to be a silent runtime failure
instead of a CHECK(false)?  Are you trying to avoid crashing the browser process
when receiving a malformed IPC from a renderer?

[rvargas (doing something else), 2010/11/11 22:35:36]

This CL intends to fix a browser crash on Linux x64, not related to IPC.

The way I understand this class is that this is the constructor to be used with
previously pickled data, so the normal use would be to distrust the data
somehow. I could certainly add a static method to perform validation previously
to instantiation of an object, but that would require us to track all current
uses of this constructor to add the call to perform the validation.

I was also a little concerned about changing the invariants about header_, but
at this point I think it is the simplest solution... it allows us to handle the
error cases with the same behavior that we have in 32 bits (just instantiate the
object, and all methods that read from it will fail), without any extra
overhead.

... and attempting to write to an invalid pickle will crash immediately, which
may not be a bad thing at this time.

 }
 
 Pickle::Pickle(const Pickle& other)
     : header_(NULL),
       header_size_(other.header_size_),
       capacity_(0),
       variable_buffer_offset_(other.variable_buffer_offset_) {
   size_t payload_size = header_size_ + other.header_->payload_size;
   bool resized = Resize(payload_size);
   CHECK(resized);  // Realloc failed.
@@ skipping 338 matching lines @@
   DCHECK(header_size <= static_cast<size_t>(kPayloadUnit));
 
   const Header* hdr = reinterpret_cast<const Header*>(start);
   const char* payload_base = start + header_size;
   const char* payload_end = payload_base + hdr->payload_size;
   if (payload_end < payload_base)
     return NULL;
 
   return (payload_end > end) ? NULL : payload_end;
 }