GitLab

This fixes a bug where opcode count was incremented (+=)
rather than set when opcodes were serialized into backing memory.

In turn this allow todoing the use of a uint32_t for low level policy
|options_| internal storage.

policy_engine_opcodes.h:243
// TODO(cpu): Making |options_| a uint32_t would avoid casting, but causes
// test failures.  Somewhere code is relying on the size of this struct.
// http://crbug.com/420296

It turns out that the test failure occurred not because of a structure
being the wrong size. Instead when policy_low_level.cc
LowLevelPolicy::Done() was called twice (because a rule was
changed or added) the memory to which rules were stored was not
zeroed before filling the memory.
This meant while most memory was rewritten with
new values the count of opcodes was instead incremented.

It's not entirely clear to me why this did not break before as
this resulted in an incorrect opcode count for any case where
::Done() was called twice but my hypothesis is:

a) ::Done() was not called twice in production nor in most tests.
b) The memory allocated for policies is much larger than required
so no sanitizers would locate a OOB read or write.
c) The alignment of the structures with a uint16_t for |options_|
caused the rule engine, when attempting to read twice as many
rules as required, to interpret the 'next' rule as a
OP_ALWAYS_FALSE and so return something sensible in these
cases. This changed when the size of the value, and packing,
stretched the size of the PolicyOpcode structure, so that when
reading past the end of the rules an invalid opcode type of
0x10 would be read (the next entry's count) which resulted
in the test failing.

Bug: 420296
Change-Id: I806a70d66fba9d4cd831df64658ded2c9545be9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1867476
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707525}