Implicit error propagation was originally used because it was convenient
and made it easy to just bubble up errors via the ? operator without
thinking. However, there have been too many situations where this
resulted in error messages that were completely useless in
troubleshooting the problem. "I/O error", even with a specific reason
attached, is useless where there are potentially hundreds of operations
where I/O can fail.
I no longer think implicit error propagation is a good idea, so this
commit removes every single use of #[from] in every custom error type.
There are now many more error variants, allowing more context to be
attached to the underlying errors.
Previously, it was easy to encounter error messages like:
Caused by:
0: Failed to patch payload: payload.bin
1: Failed to patch boot images: boot, init_boot, vendor_boot
2: Boot image error
3: I/O error
4: failed to fill whole buffer
This is a terrible error message because it doesn't mention which of the
3 boot images failed to parse, nor does it mention during which I/O
operation it encountered EOF. With this commit, this sort of information
is now included. For example, if the boot image happened to be truncated
in the middle of the ramdisk, the error message would now be:
Caused by:
0: Failed to patch payload: payload.bin
1: Failed to patch boot images: boot, init_boot, vendor_boot
2: Failed to load boot image: init_boot
3: Failed to read boot image data: Boot::V3::ramdisk
4: failed to fill whole buffer
Changes:
* Remove all uses of #[from] from thiserror-derived error types.
* Errors during parsing and serialization of RSA private keys, RSA
public keys, and X509 certificates now include the file path.
* Removed unnecessary uses of BufReader and BufWriter when reading and
writing RSA private keys, RSA public keys, and X509 certificates,
since they need to be fully read into memory anyway.
* Use ReadFixedSizeExt instead of read_exact() where possible.
* Use &'static str instead of String in error fields where all possible
values are known at compile-time to avoid unnecessary heap allocation.
* Use ok_or() instead of ok_or_else() to construct errors when the error
variant uses known data and does not require heap allocation.
* Using DebugString instead of String in error variants that store a
preformatted debug string.
While working on this commit, an unrelated bug was found and fixed:
* Fix vendor v4 boot images that were truncated within the padding
following the bootconfig section being treated as valid.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This makes the type potentially more useful outside of just our current
use case of just error messages. Ranges where the starting value is
exclusive are no longer supported, but weren't used anyway.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
The bzip2-rs library now has a new maintainer (same folks that maintain
the sudo-rs project). Version 0.5.0 was recently released, which
includes some much needed bug fixes, but is missing one final one, so we
still need to keep our fork for now.
This commit also switches the backend implementation from the official C
implementation to a Rust implementation maintained by the bzip2-rs
folks. This leaves xz as the only C dependency remaining in avbroot.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Previously, when patching with a Magisk version newer than the latest
supported version and using --ignore-magisk-warnings, the upper bound of
VER_PREINIT_DEVICE and VER_XZ_BACKUP would prevent those features from
being used. It makes more sense to assume that a newer version supports
all the same features as the latest supported version instead.
Issue: #393
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
MagiskRootPatcher was previously assuming that there is a preinit device
value if the Magisk version requires it.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
* Remove the ReadString trait and perform UTF-8 conversions explicitly.
UTF-8 conversion errors are now individual errors instead of being
hidden in an Io error.
* Add new IntOutOfBounds errors that include the actual field value and
the range of values that are valid for the field. Integer casting also
uses this error type where possible.
* Replace remaining uses of FieldOutOfBounds with IntOverflow, which is
returned when checked arithmetic fails. For code simplicity, it does
not store intermediate values.
* Replace ReadFieldError and WriteFieldError with the generic Io error
to simplify the code. They were used inconsistently anyway.
* Use fully qualified field names in avb and bootimage error messages
since the same name frequently appears in multiple structs.
* Replace InvalidFieldValue with more specific errors in bootimage.
* Replace all stringly-typed errors in lp and sparse with (very)
specific errors.
* Fix the wording in a number of errors.
* Add new ReadFixedSizeExt trait for reading fixed size arrays and vecs
from Read types.
* Remove some fallible casts from u32 to usize and from usize to u64.
avbroot only supports 32-bit and 64-bit systems anyway.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
avbroot will not support patching these internal OTAs because it
requires modifying filesystems and creating incremental OTAs. However,
it should be possible to do this manually if someone really wants to try
out a 16K page size kernel.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Neither of the two new fields need any special handling in avbroot. This
just allows us to preserve the values from the original OTA.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This commit also fixes a bug where avb::Header::release_string was
allowed to take the full 48-bytes, which was incorrect because libavb
expects the field to be NULL-terminated. This was not a problem in
practice because the release string is usually short and even if it
wasn't, the 80 reserved bytes that immediately follow it are all zeros.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
The official AOSP implementation does, so we should too. This allows
unpacking Samsung's sparse images, which have an extra 4 bytes in both
the file header and chunk headers.
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Older OTAs created before the payload metadata format supported EC
signatures will not have the `unpadded_signature_size` field set. In
this case, we'll just use the full length of `data`, which is what
update_engine also does.
Issue: #366
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
While patched OTAs produced by avbroot always include the protobuf
version of the OTA metadata, the original OTA may not. Verifying those
with `avbroot ota verify` is a valid use case.
Issue: #366
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Andrew Gunnerson