update_payload: Move minor version checks to CheckOperation.
Various small fixes. Move checks for operation and minor version
compatibility to CheckOperation.
BUG=none
TEST=`./checker_unittest.py` and running paycheck on payloads.
Change-Id: I6abd0ec200c1d4d885b09dfb84ee7e6cfad4172c
Reviewed-on: https://chromium-review.googlesource.com/254345
Reviewed-by: Allie Wood <alliewood@chromium.org>
Commit-Queue: Allie Wood <alliewood@chromium.org>
Trybot-Ready: Allie Wood <alliewood@chromium.org>
Tested-by: Allie Wood <alliewood@chromium.org>
diff --git a/host/lib/update_payload/checker.py b/host/lib/update_payload/checker.py
index 0bc62ad..48b18e6 100644
--- a/host/lib/update_payload/checker.py
+++ b/host/lib/update_payload/checker.py
@@ -494,6 +494,34 @@
'%s (%d) <= (num %sblocks - 1 (%d)) * block_size (%d).' %
(length_name, length, block_name or '', num_blocks - 1, block_size))
+ def _CheckMinorVersion(self, report, minor_version, payload_type):
+ """Checks that the minor version matches the payload type.
+
+ Args:
+ report: The report object to add to.
+ minor_version: The minor version of the payload.
+ payload_type: The type of payload (full or delta).
+
+ Raises:
+ error.PayloadError if any of the checks fails.
+ """
+ report.AddField('minor version', minor_version)
+
+ if minor_version == 0:
+ # Minor version 0 implies a full payload.
+ if payload_type != _TYPE_FULL:
+ raise error.PayloadError(
+ 'Minor version 0 not compatible with payload type: %s.'
+ % payload_type)
+ elif minor_version in (1, 2):
+ # Minor version 1 or 2 implies a delta payload.
+ if payload_type != _TYPE_DELTA:
+ raise error.PayloadError(
+ 'Minor version %d not compatible with payload type: %s.'
+ % (minor_version, payload_type))
+ else:
+ raise error.PayloadError('Unsupported minor version: %d' % minor_version)
+
def _CheckManifest(self, report, rootfs_part_size=0, kernel_part_size=0):
"""Checks the payload manifest.
@@ -802,7 +830,6 @@
"""Specific checks for SOURCE_COPY.
Args:
- op: The operation object from the manifest.
data_offset: The offset of a data blob for the operation.
total_src_blocks: Total number of blocks in src_extents.
total_dst_blocks: Total number of blocks in dst_extents.
@@ -908,22 +935,23 @@
(op_name, data_offset, prev_data_offset))
# Type-specific checks.
+ minor_version = self.payload.manifest.minor_version
if op.type in (common.OpType.REPLACE, common.OpType.REPLACE_BZ):
self._CheckReplaceOperation(op, data_length, total_dst_blocks, op_name)
- elif self.payload_type == _TYPE_FULL:
- raise error.PayloadError('%s: non-REPLACE operation in a full payload.' %
- op_name)
- elif op.type == common.OpType.MOVE:
+ elif op.type == common.OpType.MOVE and minor_version == 1:
self._CheckMoveOperation(op, data_offset, total_src_blocks,
total_dst_blocks, op_name)
- elif op.type in (common.OpType.BSDIFF, common.OpType.SOURCE_BSDIFF):
+ elif op.type == common.OpType.BSDIFF and minor_version == 1:
self._CheckBsdiffOperation(data_length, total_dst_blocks, op_name)
- elif op.type == common.OpType.SOURCE_COPY:
+ elif op.type == common.OpType.SOURCE_COPY and minor_version == 2:
self._CheckSourceCopyOperation(data_offset, total_src_blocks,
total_dst_blocks, op_name)
+ elif op.type == common.OpType.SOURCE_BSDIFF and minor_version == 2:
+ self._CheckBsdiffOperation(data_length, total_dst_blocks, op_name)
else:
- assert False, 'cannot get here'
-
+ raise error.PayloadError(
+ 'Operation %s (type %d) not allowed in minor version %d' %
+ (op_name, op.type, minor_version))
return data_length if data_length is not None else 0
def _SizeToNumBlocks(self, size):
@@ -1056,19 +1084,6 @@
'%s: not all blocks written exactly once during full update.' %
base_name)
- # Check: SOURCE_COPY and SOURCE_BSDIFF ops shouldn't be in minor version 1.
- if (self.payload.manifest.minor_version == 1 and
- (op_counts[common.OpType.SOURCE_COPY] or
- op_counts[common.OpType.SOURCE_BSDIFF])):
- raise error.PayloadError(
- 'SOURCE_COPY/SOURCE_BSDIFF not allowed with minor version 1.')
-
- # Check: MOVE and BSDIFF ops shouldn't be in minor version 2.
- if (self.payload.manifest.minor_version == 2 and
- (op_counts[common.OpType.MOVE] or op_counts[common.OpType.BSDIFF])):
- raise error.PayloadError(
- 'MOVE/BSDIFF not allowed with minor version 2.')
-
return total_data_used
def _CheckSignatures(self, report, pubkey_file_name):
@@ -1122,36 +1137,6 @@
raise error.PayloadError('Unknown signature version (%d).' %
sig.version)
- def _CheckMinorVersion(self, report, minor_version, payload_type):
- """Checks that the minor version matches the payload type.
-
- Args:
- report: The report object to add to.
- minor_version: The minor version of the payload.
- payload_type: The type of payload (full or delta).
-
- Raises:
- error.PayloadError if any of the checks fails.
- """
- report.AddField('minor version', minor_version)
-
- # Minor version 0 implies a full payload.
- if minor_version == 0:
- if payload_type != _TYPE_FULL:
- raise error.PayloadError(
- 'Minor version 0 not compatible with payload type: %s.'
- % payload_type)
-
- # Minor version 1 or 2 implies a delta payload.
- elif minor_version == 1 or minor_version == 2:
- if payload_type != _TYPE_DELTA:
- raise error.PayloadError(
- 'Minor version %d not compatible with payload type: %s.'
- % (minor_version, payload_type))
-
- else:
- raise error.PayloadError('Unsupported minor version: %d' % minor_version)
-
def Run(self, pubkey_file_name=None, metadata_sig_file=None,
rootfs_part_size=0, kernel_part_size=0, report_out_file=None):
"""Checker entry point, invoking all checks.
diff --git a/host/lib/update_payload/checker_unittest.py b/host/lib/update_payload/checker_unittest.py
index 442fcaa..c07819a 100755
--- a/host/lib/update_payload/checker_unittest.py
+++ b/host/lib/update_payload/checker_unittest.py
@@ -782,7 +782,7 @@
fail_mismatched_data_offset_length,
fail_missing_dst_extents, fail_src_length,
fail_dst_length, fail_data_hash,
- fail_prev_data_offset):
+ fail_prev_data_offset, fail_bad_minor_version):
"""Parametric testing of _CheckOperation().
Args:
@@ -800,6 +800,7 @@
fail_dst_length: Make dst length inconsistent.
fail_data_hash: Tamper with the data blob hash.
fail_prev_data_offset: Make data space uses incontiguous.
+ fail_bad_minor_version: Make minor version incompatible with op.
"""
op_type = _OpTypeByName(op_type_name)
@@ -834,6 +835,13 @@
self.NewExtentList((0, 16)))
total_src_blocks = 16
+ if op_type in (common.OpType.REPLACE, common.OpType.REPLACE_BZ):
+ payload.manifest.minor_version = 0
+ elif op_type in (common.OpType.MOVE, common.OpType.BSDIFF):
+ payload.manifest.minor_version = 2 if fail_bad_minor_version else 1
+ elif op_type in (common.OpType.SOURCE_COPY, common.OpType.SOURCE_BSDIFF):
+ payload.manifest.minor_version = 1 if fail_bad_minor_version else 2
+
if op_type not in (common.OpType.MOVE, common.OpType.SOURCE_COPY):
if not fail_mismatched_data_offset_length:
op.data_length = 16 * block_size - 8
@@ -886,7 +894,8 @@
should_fail = (fail_src_extents or fail_dst_extents or
fail_mismatched_data_offset_length or
fail_missing_dst_extents or fail_src_length or
- fail_dst_length or fail_data_hash or fail_prev_data_offset)
+ fail_dst_length or fail_data_hash or fail_prev_data_offset or
+ fail_bad_minor_version)
args = (op, 'foo', is_last, old_block_counters, new_block_counters,
old_part_size, new_part_size, prev_data_offset, allow_signature,
blob_hash_counts)
@@ -959,54 +968,6 @@
self.assertEqual(rootfs_data_length,
payload_checker._CheckOperations(*args))
- def testCheckOperationsMinorVersion(self):
- """Tests _CheckOperations; checks op compatibility with minor version.
-
- SOURCE_COPY and SOURCE_BSDIFF operations are not supported in payloads
- with delta minor version 1. MOVE and BSDIFF operations are not supported
- in payloads with delta minor version 2.
- """
- # Create test objects and parameters.
- payload_checker = checker.PayloadChecker(self.MockPayload())
- report = checker._PayloadReport()
- rootfs_part_size = test_utils.MiB(8)
-
- FakeOp = collections.namedtuple('FakeOp', ['type'])
- args = ([FakeOp(common.OpType.SOURCE_COPY),
- FakeOp(common.OpType.SOURCE_BSDIFF)],
- report, 'foo', 0, rootfs_part_size, rootfs_part_size, 0, False)
-
- try:
- # Mock _CheckOperation() so it will pass.
- self.mox.StubOutWithMock(payload_checker, '_CheckOperation')
- payload_checker._CheckOperation(mox.IgnoreArg(), mox.IgnoreArg(),
- mox.IgnoreArg(), mox.IgnoreArg(),
- mox.IgnoreArg(), mox.IgnoreArg(),
- mox.IgnoreArg(), mox.IgnoreArg(),
- mox.IgnoreArg(), mox.IgnoreArg()
- ).MultipleTimes().AndReturn(0)
- self.mox.ReplayAll()
-
- # Fail, minor version 1 should not allow SOURCE_COPY or SOURCE_BSDIFF.
- payload_checker.payload.manifest.minor_version = 1
- self.assertRaises(update_payload.PayloadError,
- payload_checker._CheckOperations,
- *args)
-
- # Pass, minor version 2 should allow these operations.
- payload_checker.payload.manifest.minor_version = 2
- self.assertEquals(payload_checker._CheckOperations(*args), 0)
-
- # Fail, minor version 2 should not allow MOVE or BSDIFF.
- args = ([FakeOp(common.OpType.MOVE), FakeOp(common.OpType.BSDIFF)],
- report, 'foo', 0, rootfs_part_size, rootfs_part_size, 0, False)
- self.assertRaises(update_payload.PayloadError,
- payload_checker._CheckOperations,
- *args)
-
- finally:
- self.mox.UnsetStubs()
-
def DoCheckSignaturesTest(self, fail_empty_sigs_blob, fail_missing_pseudo_op,
fail_mismatched_pseudo_op, fail_sig_missing_fields,
fail_unknown_sig_version, fail_incorrect_sig):
@@ -1167,13 +1128,14 @@
fail_mismatched_data_offset_length,
fail_missing_dst_extents, fail_src_length,
fail_dst_length, fail_data_hash,
- fail_prev_data_offset):
+ fail_prev_data_offset, fail_bad_minor_version):
"""Returns True iff the combination of arguments represents a valid test."""
op_type = _OpTypeByName(op_type_name)
- # REPLACE/REPLACE_BZ operations don't read data from src partition.
+ # REPLACE/REPLACE_BZ operations don't read data from src partition. They are
+ # compatible with all valid minor versions, so we don't need to check that.
if (op_type in (common.OpType.REPLACE, common.OpType.REPLACE_BZ) and (
- fail_src_extents or fail_src_length)):
+ fail_src_extents or fail_src_length or fail_bad_minor_version)):
return False
# MOVE and SOURCE_COPY operations don't carry data.
@@ -1272,7 +1234,8 @@
'fail_src_length': (True, False),
'fail_dst_length': (True, False),
'fail_data_hash': (True, False),
- 'fail_prev_data_offset': (True, False)},
+ 'fail_prev_data_offset': (True, False),
+ 'fail_bad_minor_version': (True, False)},
validate_func=ValidateCheckOperationTest)
# Add all _CheckOperations() test cases.
