Merge "Add some more checkings to the DiffEncoder."

diff_encoder.cc

@@ -32,14 +32,18 @@ bool DiffEncoder::AddControlEntry(const ControlEntry& entry) {
     return false;
   }
 
-  if (written_output_ + entry.diff_size + entry.extra_size > new_size_) {
+  // entry.diff_size + entry.extra_size don't overflow in uint64_t since we
+  // checked the kMaxEncodedUint64Value limit before.
+  if (entry.diff_size + entry.extra_size > new_size_ - written_output_) {
     LOG(ERROR) << "Wrote more output than the declared new_size" << endl;
     return false;
   }
 
   if (entry.diff_size > 0 &&
-      (old_pos_ < 0 || static_cast<uint64_t>(old_pos_) >= old_size_)) {
-    LOG(ERROR) << "The pointer in the old stream (" << old_pos_
+      (old_pos_ < 0 ||
+       static_cast<uint64_t>(old_pos_) + entry.diff_size > old_size_)) {
+    LOG(ERROR) << "The pointer in the old stream [" << old_pos_ << ", "
+               << (static_cast<uint64_t>(old_pos_) + entry.diff_size)
                << ") is out of bounds [0, " << old_size_ << ")" << endl;
     return false;
   }

diff_encoder_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "bsdiff/diff_encoder.h"
 
+#include <memory>
 #include <vector>
 
 #include <gtest/gtest.h>
@@ -25,7 +26,13 @@ namespace bsdiff {
 
 class DiffEncoderTest : public testing::Test {
  protected:
-  void SetUp() { EXPECT_TRUE(fake_patch_.Init()); }
+  void SetUp() {
+    EXPECT_TRUE(fake_patch_.Init());
+    // By default, set the encoder to kHelloWorld to kHelloWorld.
+    diff_encoder_.reset(new DiffEncoder(&fake_patch_, kHelloWorld,
+                                        sizeof(kHelloWorld), kHelloWorld,
+                                        sizeof(kHelloWorld)));
+  }
 
   FakePatchWriter fake_patch_;
   std::unique_ptr<DiffEncoder> diff_encoder_;
@@ -59,9 +66,6 @@ TEST_F(DiffEncoderTest, AllInExtraStreamTest) {
 }
 
 TEST_F(DiffEncoderTest, AllInDiffStreamTest) {
-  diff_encoder_.reset(new DiffEncoder(&fake_patch_, kHelloWorld,
-                                      sizeof(kHelloWorld), kHelloWorld,
-                                      sizeof(kHelloWorld)));
   EXPECT_TRUE(
       diff_encoder_->AddControlEntry(ControlEntry(sizeof(kHelloWorld), 0, 0)));
   EXPECT_TRUE(diff_encoder_->Close());
@@ -71,4 +75,36 @@ TEST_F(DiffEncoderTest, AllInDiffStreamTest) {
   EXPECT_TRUE(fake_patch_.extra_stream().empty());
 }
 
+TEST_F(DiffEncoderTest, OldPosNegativeErrorTest) {
+  // Referencing negative values in oldpos is fine, until you use them.
+  EXPECT_TRUE(diff_encoder_->AddControlEntry(ControlEntry(0, 0, -5)));
+  EXPECT_TRUE(diff_encoder_->AddControlEntry(ControlEntry(0, 0, 2)));
+  EXPECT_FALSE(diff_encoder_->AddControlEntry(ControlEntry(1, 0, 0)));
+}
+
+// Test that using an oldpos past the end of the file fails.
+TEST_F(DiffEncoderTest, OldPosTooBigErrorTest) {
+  EXPECT_TRUE(
+      diff_encoder_->AddControlEntry(ControlEntry(0, 0, sizeof(kHelloWorld))));
+  EXPECT_FALSE(diff_encoder_->AddControlEntry(ControlEntry(1, 0, 0)));
+}
+
+// Test that diffing against a section of the old file past the end of the file
+// fails.
+TEST_F(DiffEncoderTest, OldPosPlusSizeTooBigErrorTest) {
+  // The oldpos is set to a range inside the word, the we try to copy past the
+  // end of it.
+  EXPECT_TRUE(diff_encoder_->AddControlEntry(
+      ControlEntry(0, 0, sizeof(kHelloWorld) - 3)));
+  EXPECT_FALSE(
+      diff_encoder_->AddControlEntry(ControlEntry(sizeof(kHelloWorld), 0, 0)));
+}
+
+TEST_F(DiffEncoderTest, ExtraStreamTooBigErrorTest) {
+  EXPECT_TRUE(diff_encoder_->AddControlEntry(ControlEntry(3, 0, 0)));
+  // This writes too many bytes in the stream because we already have 3 bytes.
+  EXPECT_FALSE(
+      diff_encoder_->AddControlEntry(ControlEntry(0, sizeof(kHelloWorld), 0)));
+}
+
 }  // namespace bsdiff