[Zucchini] Format change: PatchElementHeader: Group fields by "old" and "new".

Previously PatchElementHeader members were ordered by:
  [old_offset, new_offset, old_length, new_length].
This CL changes the order to
  [old_offset, old_length, new_offset, new_length]
to be consistent with PatchHeader's grouping by "old" and "new".

Also improve comments in patch_read_write_unittest.cc, and fix
input value in PatchElementTest.WrongExtraData.

Change-Id: I0dab56b7de1ba99f0a2f686dfebdf422a9d1c3b0
Reviewed-on: https://chromium-review.googlesource.com/1043073
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556076}

components/zucchini/README.md

@@ -209,8 +209,8 @@ PatchElement self contained.
 Name | Format | Description
 --- | --- | ---
 old_offset | uint32 | Starting offset of the element in old file.
-new_offset | uint32 | Starting offset of the element in new file.
 old_length | uint32 | Length of the element in old file.
+new_offset | uint32 | Starting offset of the element in new file.
 new_length | uint32 | Length of the element in new file.
 exe_type | uint32 | Executable type for this unit, see `enum ExecutableType`.
 

components/zucchini/patch_read_write_unittest.cc

@@ -57,30 +57,31 @@ void TestSerialize(const ByteVector& expected, const T& value) {
 
 ByteVector CreatePatchElement() {
   return {
+      // PatchElementHeader
       0x01, 0, 0, 0,       // old_offset
-      0x03, 0,   0,   0,    // new_offset
       0x51, 0, 0, 0,       // old_length
+      0x03, 0, 0, 0,       // new_offset
       0x13, 0, 0, 0,       // new_length
-      'P',  'x', '8', '6',  // EXE_TYPE_WIN32_X86
-
+      'P', 'x', '8', '6',  // exe_type = EXE_TYPE_WIN32_X86
+      // EquivalenceSource
       1, 0, 0, 0,  // src_skip size
       0x10,        // src_skip content
       1, 0, 0, 0,  // dst_skip size
       0x00,        // dst_skip content
       1, 0, 0, 0,  // copy_count size
       0x12,        // copy_count content
-
+      // ExtraDataSource
       1, 0, 0, 0,  // extra_data size
       0x13,        // extra_data content
-
+      // RawDeltaSource
       1, 0, 0, 0,  // raw_delta_skip size
       0x14,        // raw_delta_skip content
       1, 0, 0, 0,  // raw_delta_diff size
       0x15,        // raw_delta_diff content
-
+      // ReferenceDeltaSource
       1, 0, 0, 0,  // reference_delta size
       0x16,        // reference_delta content
-
+      // PatchElementReader
       2, 0, 0, 0,  // pool count
       0,           // pool_tag
       1, 0, 0, 0,  // extra_targets size
@@ -93,11 +94,12 @@ ByteVector CreatePatchElement() {
 
 ByteVector CreateElementMatch() {
   return {
+      // PatchElementHeader
       0x01, 0,   0,   0,    // old_offset
-      0x03, 0,   0,   0,    // new_offset
       0x02, 0,   0,   0,    // old_length
+      0x03, 0,   0,   0,    // new_offset
       0x04, 0,   0,   0,    // new_length
-      'D',  'E', 'X', ' ',  // kExeTypeDex
+      'D',  'E', 'X', ' ',  // exe_type = kExeTypeDex
   };
 }
 
@@ -149,7 +151,8 @@ TEST(PatchTest, ParseElementMatchNoLength) {
   // Set old_length to 0 to trigger an error.
   {
     ByteVector data = CreateElementMatch();
-    ModifyByte(8U, 0x02, 0x00, &data);  // Make the old_length = 0.
+    // old_length := 0.
+    ModifyByte(offsetof(PatchElementHeader, old_length), 0x02, 0x00, &data);
     BufferSource buffer_source(data.data(), data.size());
     ElementMatch element_match = {};
     EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
@@ -157,7 +160,8 @@ TEST(PatchTest, ParseElementMatchNoLength) {
   // Set new_length to 0 to trigger an error.
   {
     ByteVector data = CreateElementMatch();
-    ModifyByte(12U, 0x04, 0x00, &data);  // Make the new_length = 0.
+    // new_length := 0.
+    ModifyByte(offsetof(PatchElementHeader, new_length), 0x04, 0x00, &data);
     BufferSource buffer_source(data.data(), data.size());
     ElementMatch element_match = {};
     EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
@@ -165,8 +169,10 @@ TEST(PatchTest, ParseElementMatchNoLength) {
   // Set both new_length and old_length to 0 to trigger an error.
   {
     ByteVector data = CreateElementMatch();
-    ModifyByte(8U, 0x02, 0x00, &data);   // Make the old_length = 0.
-    ModifyByte(12U, 0x04, 0x00, &data);  // Make the new_length = 0.
+    // old_length := 0.
+    ModifyByte(offsetof(PatchElementHeader, old_length), 0x02, 0x00, &data);
+    // new_length := 0.
+    ModifyByte(offsetof(PatchElementHeader, new_length), 0x04, 0x00, &data);
     BufferSource buffer_source(data.data(), data.size());
     ElementMatch element_match = {};
     EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
@@ -245,6 +251,7 @@ TEST(PatchTest, SerializeBufferTooSmall) {
 
 TEST(EquivalenceSinkSourceTest, Empty) {
   ByteVector data = {
+      // EquivalenceSource
       0, 0, 0, 0,  // src_skip size
       0, 0, 0, 0,  // dst_skip size
       0, 0, 0, 0,  // copy_count size
@@ -260,6 +267,7 @@ TEST(EquivalenceSinkSourceTest, Empty) {
 
 TEST(EquivalenceSourceSinkTest, Normal) {
   ByteVector data = {
+      // EquivalenceSource
       2, 0, 0, 0,  // src_skip size
       6, 7,        // src_skip content
       2, 0, 0, 0,  // dst_skip size
@@ -294,6 +302,7 @@ TEST(EquivalenceSourceSinkTest, Normal) {
 
 TEST(ExtraDataSourceSinkTest, Empty) {
   ByteVector data = {
+      // ExtraDataSource
       0, 0, 0, 0,  // extra_data size
   };
   ExtraDataSource extra_data_source = TestInitialize<ExtraDataSource>(&data);
@@ -306,6 +315,7 @@ TEST(ExtraDataSourceSinkTest, Empty) {
 
 TEST(ExtraDataSourceSinkTest, Normal) {
   ByteVector data = {
+      // ExtraDataSource
       5, 0, 0, 0,     // extra_data size
       1, 2, 3, 4, 5,  // extra_data content
   };
@@ -339,6 +349,7 @@ TEST(ExtraDataSourceSinkTest, Normal) {
 
 TEST(RawDeltaSourceSinkTest, Empty) {
   ByteVector data = {
+      // RawDeltaSource
       0, 0, 0, 0,  // raw_delta_skip size
       0, 0, 0, 0,  // raw_delta_diff size
   };
@@ -352,6 +363,7 @@ TEST(RawDeltaSourceSinkTest, Empty) {
 
 TEST(RawDeltaSinkSourceSinkTest, Normal) {
   ByteVector data = {
+      // RawDeltaSource
       3,  0,  0,   0,  // raw_delta_skip size
       1,  3,  0,       // raw_delta_skip content
       3,  0,  0,   0,  // raw_delta_diff size
@@ -390,6 +402,7 @@ TEST(RawDeltaSinkSourceSinkTest, Normal) {
 
 TEST(RawDeltaSourceSinkTest, InvalidContent) {
   ByteVector data = {
+      // RawDeltaSource
       2, 0, 0, 0,  // raw_delta_skip size
       1, 3,        // raw_delta_skip content
       2, 0, 0, 0,  // raw_delta_diff size
@@ -402,6 +415,7 @@ TEST(RawDeltaSourceSinkTest, InvalidContent) {
 
 TEST(ReferenceDeltaSourceSinkTest, Empty) {
   ByteVector data = {
+      // ReferenceDeltaSource
       0, 0, 0, 0,  // reference_delta size
   };
   ReferenceDeltaSource reference_delta_source =
@@ -415,6 +429,7 @@ TEST(ReferenceDeltaSourceSinkTest, Empty) {
 
 TEST(ReferenceDeltaSourceSinkTest, Normal) {
   ByteVector data = {
+      // ReferenceDeltaSource
       2,  0,  0, 0,  // reference_delta size
       84, 47,        // reference_delta content
   };
@@ -443,6 +458,7 @@ TEST(ReferenceDeltaSourceSinkTest, Normal) {
 
 TEST(TargetSourceSinkTest, Empty) {
   ByteVector data = {
+      // TargetSource
       0, 0, 0, 0,  // extra_targets size
   };
   TargetSource target_source = TestInitialize<TargetSource>(&data);
@@ -455,6 +471,7 @@ TEST(TargetSourceSinkTest, Empty) {
 
 TEST(TargetSourceSinkTest, Normal) {
   ByteVector data = {
+      // TargetSource
       2, 0, 0, 0,  // extra_targets size
       3, 1,        // extra_targets content
   };
@@ -535,39 +552,43 @@ TEST(PatchElementTest, Normal) {
 }
 
 TEST(PatchElementTest, BadEquivalence) {
-  // If the "old" element is too small the test should fail.
+  // If the "old" element is too small then the test should fail.
   {
     ByteVector data = CreatePatchElement();
-    ModifyByte(8, 0x51, 0x04, &data);  // old_length (too small)
+    // old_length := 0x4 (too small).
+    ModifyByte(offsetof(PatchElementHeader, old_length), 0x51, 0x04, &data);
     TestInvalidInitialize<PatchElementReader>(&data);
   }
 
-  // If the "new" element is too small the test should fail.
+  // If the "new" element is too small then the test should fail.
   {
     ByteVector data = CreatePatchElement();
-    ModifyByte(12, 0x13, 0x05, &data);  // new_length (too small)
+    // new_length := 0x5 (too small).
+    ModifyByte(offsetof(PatchElementHeader, new_length), 0x13, 0x05, &data);
     TestInvalidInitialize<PatchElementReader>(&data);
   }
 }
 
 TEST(PatchElementTest, WrongExtraData) {
-  // Make "new" too large so there is insufficient extra data to cover the
-  // image.
+  // Make "new" too large so insufficient extra data exists to cover the image.
   {
     ByteVector data = CreatePatchElement();
-    ModifyByte(12, 0x13, 0x14, &data);  // new_length (too large)
+    // new_length := 0x14 (too large).
+    ModifyByte(offsetof(PatchElementHeader, new_length), 0x13, 0x14, &data);
     TestInvalidInitialize<PatchElementReader>(&data);
   }
   // Make "new" too small so there is too much extra data.
   {
     ByteVector data = CreatePatchElement();
-    ModifyByte(12, 0x13, 0x14, &data);  // new_length (too small)
+    // new_length := 0x12 (too small).
+    ModifyByte(offsetof(PatchElementHeader, new_length), 0x13, 0x12, &data);
     TestInvalidInitialize<PatchElementReader>(&data);
   }
 }
 
 TEST(EnsemblePatchTest, RawPatch) {
   ByteVector data = {
+      // PatchHeader
       0x5A, 0x75, 0x63, 0x00,  // magic
       0x10, 0x32, 0x54, 0x76,  // old_size
       0x00, 0x11, 0x22, 0x33,  // old_crc
@@ -576,19 +597,25 @@ TEST(EnsemblePatchTest, RawPatch) {
 
       1, 0, 0, 0,  // number of element
 
+      // PatchElementHeader
       0x01, 0, 0, 0,       // old_offset
-      0x00, 0,    0,    0,    // new_offset
       0x02, 0, 0, 0,       // old_length
+      0x00, 0, 0, 0,       // new_offset
       0x01, 0, 0, 0,       // new_length
-      'P',  'x',  '8',  '6',  // EXE_TYPE_WIN32_X86
+      'P', 'x', '8', '6',  // exe_type = EXE_TYPE_WIN32_X86
+      // EquivalenceSource
       0, 0, 0, 0,  // src_skip size
       0, 0, 0, 0,  // dst_skip size
       0, 0, 0, 0,  // copy_count size
+      // ExtraDataSource
       0x01, 0, 0, 0,  // extra_data size
       0x04,           // extra_data content
+      // RawDeltaSource
       0, 0, 0, 0,  // raw_delta_skip size
       0, 0, 0, 0,  // raw_delta_diff size
+      // ReferenceDeltaSource
       0, 0, 0, 0,  // reference_delta size
+      // PatchElementReader
       0, 0, 0, 0,  // pool count
   };
 
@@ -619,6 +646,7 @@ TEST(EnsemblePatchTest, RawPatch) {
 
 TEST(EnsemblePatchTest, CheckFile) {
   ByteVector data = {
+      // PatchHeader
       0x5A, 0x75, 0x63, 0x00,  // magic
       0x05, 0x00, 0x00, 0x00,  // old_size
       0xDF, 0x13, 0xE4, 0x10,  // old_crc
@@ -627,19 +655,25 @@ TEST(EnsemblePatchTest, CheckFile) {
 
       1, 0, 0, 0,  // number of element
 
+      // PatchElementHeader
       0x01, 0, 0, 0,       // old_offset
-      0x00, 0,    0,    0,    // new_offset
       0x02, 0, 0, 0,       // old_length
+      0x00, 0, 0, 0,       // new_offset
       0x03, 0, 0, 0,       // new_length
-      'P',  'x',  '8',  '6',  // EXE_TYPE_WIN32_X86
+      'P', 'x', '8', '6',  // exe_type = EXE_TYPE_WIN32_X86
+      // EquivalenceSource
       0, 0, 0, 0,  // src_skip size
       0, 0, 0, 0,  // dst_skip size
       0, 0, 0, 0,  // copy_count size
+      // ExtraDataSource
       0x03, 0, 0, 0,  // extra_data size
       'A', 'B', 'C',  // extra_data content
+      // RawDeltaSource
       0, 0, 0, 0,  // raw_delta_skip size
       0, 0, 0, 0,  // raw_delta_diff size
+      // ReferenceDeltaSource
       0, 0, 0, 0,  // reference_delta size
+      // PatchElementReader
       0, 0, 0, 0,  // pool count
   };
 
@@ -660,6 +694,7 @@ TEST(EnsemblePatchTest, CheckFile) {
 
 TEST(EnsemblePatchTest, InvalidMagic) {
   ByteVector data = {
+      // PatchHeader
       0x42, 0x42, 0x42, 0x00,  // magic
       0x10, 0x32, 0x54, 0x76,  // old_size
       0x00, 0x11, 0x22, 0x33,  // old_crc
@@ -668,18 +703,24 @@ TEST(EnsemblePatchTest, InvalidMagic) {
 
       1, 0, 0, 0,  // number of element
 
+      // PatchElementHeader
       0x01, 0, 0, 0,       // old_offset
-      0x00, 0,    0,    0,    // new_offset
       0x02, 0, 0, 0,       // old_length
+      0x00, 0, 0, 0,       // new_offset
       0x03, 0, 0, 0,       // new_length
-      'P',  'x',  '8',  '6',  // EXE_TYPE_WIN32_X86
+      'P', 'x', '8', '6',  // exe_type = EXE_TYPE_WIN32_X86
+      // EquivalenceSource
       0, 0, 0, 0,  // src_skip size
       0, 0, 0, 0,  // dst_skip size
       0, 0, 0, 0,  // copy_count size
+      // ExtraDataSource
       0, 0, 0, 0,  // extra_data size
+      // RawDeltaSource
       0, 0, 0, 0,  // raw_delta_skip size
       0, 0, 0, 0,  // raw_delta_diff size
+      // ReferenceDeltaSource
       0, 0, 0, 0,  // reference_delta size
+      // PatchElementReader
       0, 0, 0, 0,  // pool count
   };
 

components/zucchini/patch_reader.cc

@@ -38,8 +38,8 @@ bool ParseElementMatch(BufferSource* source, ElementMatch* element_match) {
   // Caveat: Element offsets and lengths can still be invalid (e.g., exceeding
   // archive bounds), but this will be checked later.
   element_match->old_element.offset = element_header.old_offset;
-  element_match->new_element.offset = element_header.new_offset;
   element_match->old_element.size = element_header.old_length;
+  element_match->new_element.offset = element_header.new_offset;
   element_match->new_element.size = element_header.new_length;
   element_match->old_element.exe_type = exe_type;
   element_match->new_element.exe_type = exe_type;

components/zucchini/patch_utils.h

@@ -34,20 +34,20 @@ struct PatchHeader {
 };
 
 // Sanity check.
-static_assert(sizeof(PatchHeader) == 20, "PatchHeader is 20 bytes");
+static_assert(sizeof(PatchHeader) == 20, "PatchHeader must be 20 bytes");
 
 // Header for a patch element, found at the beginning of every patch element.
 struct PatchElementHeader {
   uint32_t old_offset;
-  uint32_t new_offset;
   uint32_t old_length;
+  uint32_t new_offset;
   uint32_t new_length;
-  uint32_t exe_type;
+  uint32_t exe_type;  // ExecutableType.
 };
 
 // Sanity check.
 static_assert(sizeof(PatchElementHeader) == 20,
-              "PatchElementHeader is 28 bytes");
+              "PatchElementHeader must be 20 bytes");
 
 #pragma pack(pop)
 

components/zucchini/patch_writer.cc

@@ -23,10 +23,10 @@ bool SerializeElementMatch(const ElementMatch& element_match,
   PatchElementHeader element_header;
   element_header.old_offset =
       base::checked_cast<uint32_t>(element_match.old_element.offset);
-  element_header.new_offset =
-      base::checked_cast<uint32_t>(element_match.new_element.offset);
   element_header.old_length =
       base::checked_cast<uint32_t>(element_match.old_element.size);
+  element_header.new_offset =
+      base::checked_cast<uint32_t>(element_match.new_element.offset);
   element_header.new_length =
       base::checked_cast<uint32_t>(element_match.new_element.size);
   element_header.exe_type = element_match.exe_type();