Harden error paths across parsers, backends and the build - #302
Open
igoropaniuk wants to merge 10 commits into
Open
Harden error paths across parsers, backends and the build#302igoropaniuk wants to merge 10 commits into
igoropaniuk wants to merge 10 commits into
Conversation
When json_parse_array() or json_parse_object() failed to parse an element, the partially built value was released with a shallow free() rather than json_free(). Any key string or nested subtree already attached to that value was leaked. Since flashmap.json can be parsed straight from an untrusted zip archive, malformed input was enough to trigger the leak; use json_free() so the whole subtree is released. json_parse_buf() also stored its size_t length in an int, silently truncating on 64-bit hosts: a blob larger than INT_MAX would parse only a prefix, and a size in the 2-4 GiB range turned negative and made every read report EOF. Reject inputs larger than INT_MAX up front so the parser never operates on a truncated view. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
contents_load() dereferenced firehose_alloc_op() without checking for NULL when building the per-selector configure op; handle the allocation failure by unwinding through the existing cleanup path. contents_find_programmers() also leaked the strdup'd blob name. load_sahara_image() populates blob.name, but decode_sahara_config() only frees blob.ptr, so the name outlived both the success and the hard-error paths. Release the blob with sahara_images_free() in both cases; it is NULL-safe on the already-freed ptr. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
patch_load_xml() logged an error and skipped a <patch> entry whose attributes failed to parse, but still returned 0. The caller could therefore never observe the failure, so flashing continued and qdl exited successfully with a patch silently dropped. Patch directives are typically GPT CRC fixups, so this could leave a device with a corrupt partition table while the tool reported success. Return an error instead, matching program_load_xml()'s abort-on-first-bad-tag behaviour. The now-unused patches_loaded flag is removed. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
read_op_load() skipped a malformed <read> entry with a bare free() of the op and always returned 0, so a broken read-type file was reported as success and the entry's already-allocated filename and start_sector strings were leaked. Free the op's members and return an error on a parse failure, and guard the op allocation for NULL. The include-directory fixup also overwrote read_op->filename with a fresh strdup() without freeing the original, leaking it once per resolved entry, and dereferenced a NULL filename via snprintf when the attribute was empty. Free the old string first and only apply the fixup when a filename is present. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
sim_read() copied the entire queued response into the caller's buffer using the response length, ignoring the caller's len. A read issued with a buffer smaller than the queued XML response would overflow it. Clamp the copy to the smaller of the two and, when the response does not fit, keep the remainder queued for the next read rather than dropping data. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
usb_match_edl_interface() accepted an interface on its class, subclass and protocol alone, leaving the endpoint addresses at -1 and the max packet sizes at 0 when the interface exposed no bulk endpoints. A subsequent --out-chunk-size modulo, or the first read/write modulo against a zero max packet size, was a division by zero, and the -1 endpoint address would be handed to libusb_bulk_transfer(). Reject any matching interface that lacks a bulk IN and OUT endpoint with a non-zero max packet size. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
vip_transfer_init() tested the result of open() with "!fd", but open() returns -1 on failure, so a missing or unreadable signed VIP table passed initialisation and only surfaced later mid-handshake; a legitimate fd of 0 would also have been misreported. Check for a negative fd instead. vip_gen_chunk_store() closes the digest file and clears the pointer on a write error, but a following store passed the NULL FILE* to fwrite() and vip_gen_finalize() called fclose() on it unconditionally, so a disk-full condition during --create-digest crashed instead of failing cleanly. Skip a store once the file has been closed and guard the finalize fclose(). Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
sparse_chunk_header_parse() ignored the return value of qdl_file_seek(). Seeking is unsupported on zip members (every non-trivial seek returns -1), so a sparse image programmed straight from a zip archive silently parsed the next chunk header from the wrong offset, producing confusing "unknown chunk type" errors or, for a single raw chunk, flashing the wrong bytes. Check each seek and fail the parse when it does not succeed. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
_FILE_OFFSET_BITS was defined in five source files individually, but off_t appears in shared types - struct firehose_op carries an off_t sparse_offset, and qdl_file_seek() takes an off_t - so files that did not define it (file.c, read.c, patch.c, qdl.c) disagreed on the width of off_t on 32-bit builds, giving mismatched struct layouts and a corrupt ABI between translation units. Define it once via add_project_arguments() so every unit agrees, and drop the per-file definitions. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
json_parse_number() rejected any strtod() result that set errno to ERANGE, but strtod reports ERANGE both for overflow and for underflow to a subnormal or zero. Overflow already returns +/-inf and is caught by the !isfinite() check, so the ERANGE test only served to reject spec-valid numbers such as 1e-310, which strtod returns as a finite denormal. Drop the ERANGE check (and the now unused errno handling) so underflow is accepted while overflow and malformed tokens are still rejected. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second batch of fixes from the same audit that produced #284: MEDIUM-severity hardening of error paths, resource cleanup and input validation. No behavior
changes on the happy path.
subnormal numbers instead of rejecting them on ERANGE
(prevents a division by zero and transfers on endpoint -1)
!fd->fd < 0) and make adisk-full during --create-digest fail cleanly instead of crashing