Check for integer overflow in sfntly::FontData::Bound().

Also delete dead code and cleanup some nits.

This is cl/96914065.

3 files changed, 60 insertions, 30 deletions

diff --git a/cpp/src/sfntly/data/font_data.cc b/cpp/src/sfntly/data/font_data.cc
index d2b95ea..95bee3e 100644
--- a/cpp/src/sfntly/data/font_data.cc
+++ b/cpp/src/sfntly/data/font_data.cc
@@ -14,11 +14,13 @@
  * limitations under the License.
  */
 
-#include <limits.h>
+#include "sfntly/data/font_data.h"
+
 #include <algorithm>
 #include <functional>
+#include <limits>
 
-#include "sfntly/data/font_data.h"
+#include "sfntly/port/logging.h"
 
 namespace sfntly {
 
@@ -26,21 +28,29 @@ int32_t FontData::Size() const {
   return std::min<int32_t>(array_->Size() - bound_offset_, bound_length_);
 }
 
-bool FontData::Bound(int32_t offset, int32_t length) {
-  if (offset + length > Size() || offset < 0 || length < 0)
-    return false;
-
-  bound_offset_ += offset;
+void FontData::Bound(int32_t offset, int32_t length) {
+  // Inputs should not be negative.
+  CHECK(offset >= 0);
+  CHECK(length >= 0);
+
+  // Check to make sure |bound_offset_| will not overflow.
+  CHECK(bound_offset_ <= std::numeric_limits<int32_t>::max() - offset);
+  const int32_t new_offset = bound_offset_ + offset;
+
+  if (length == GROWABLE_SIZE) {
+    // When |length| has the special value of GROWABLE_SIZE, it means the size
+    // should not have any artificial limits, thus it is just the underlying
+    // |array_|'s size. Just make sure |new_offset| is still within bounds.
+    CHECK(new_offset <= array_->Size());
+  } else {
+    // When |length| has any other value, |new_offset| + |length| points to the
+    // end of the array. Make sure that is within bounds, but use subtraction to
+    // avoid an integer overflow.
+    CHECK(new_offset <= array_->Size() - length);
+  }
+
+  bound_offset_ = new_offset;
   bound_length_ = length;
-  return true;
-}
-
-bool FontData::Bound(int32_t offset) {
-if (offset > Size() || offset < 0)
-    return false;
-
-  bound_offset_ += offset;
-  return true;
 }
 
 int32_t FontData::Length() const {
@@ -60,7 +70,7 @@ FontData::FontData(FontData* data, int32_t offset) {
   Init(data->array_);
   Bound(data->bound_offset_ + offset,
         (data->bound_length_ == GROWABLE_SIZE)
-            ? GROWABLE_SIZE : data->bound_length_ - offset);
+        ? GROWABLE_SIZE : data->bound_length_ - offset);
 }
 
 FontData::~FontData() {}
diff --git a/cpp/src/sfntly/data/font_data.h b/cpp/src/sfntly/data/font_data.h
index d02e8b7..e0e7e79 100644
--- a/cpp/src/sfntly/data/font_data.h
+++ b/cpp/src/sfntly/data/font_data.h
@@ -19,11 +19,9 @@
 
 #include <limits.h>
 
-#include <vector>
-
-#include "sfntly/port/type.h"
 #include "sfntly/data/byte_array.h"
 #include "sfntly/port/refcount.h"
+#include "sfntly/port/type.h"
 
 namespace sfntly {
 
@@ -60,16 +58,7 @@ class FontData : virtual public RefCount {
   // visible within the bounds set.
   // @param offset the start of the new bounds
   // @param length the number of bytes in the bounded array
-  // @return true if the bounding range was successful; false otherwise
-  virtual bool Bound(int32_t offset, int32_t length);
-
-  // Sets limits on the size of the FontData. This is a offset bound only so if
-  // the FontData is writable and growable then there is no limit to that growth
-  // from the bounding operation.
-  // @param offset the start of the new bounds which must be within the current
-  //        size of the FontData
-  // @return true if the bounding range was successful; false otherwise
-  virtual bool Bound(int32_t offset);
+  virtual void Bound(int32_t offset, int32_t length);
 
   // Makes a slice of this FontData. The returned slice will share the data with
   // the original <code>FontData</code>.
diff --git a/cpp/src/sfntly/port/logging.h b/cpp/src/sfntly/port/logging.h
new file mode 100644
index 0000000..1d9e319
--- /dev/null
+++ b/cpp/src/sfntly/port/logging.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2015 Google Inc. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
+#define SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
+
+#include <stdio.h>
+#include <stdlib.h>
+
+// Cheap base/logging.h knock off.
+
+#define CHECK(expr) \
+    if (!(expr)) { \
+      printf("CHECK failed\n"); \
+      abort(); \
+    }
+
+#endif  // SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_