Address memory leaks/mitigate buffer overflows#161
Merged
andersson merged 5 commits intolinux-msm:masterfrom Feb 25, 2026
Merged
Address memory leaks/mitigate buffer overflows#161andersson merged 5 commits intolinux-msm:masterfrom
andersson merged 5 commits intolinux-msm:masterfrom
Conversation
Contributor
Author
|
@lumag @andersson @konradybcio could you take a look please |
lumag
reviewed
Dec 11, 2025
andersson
requested changes
Dec 16, 2025
Collaborator
andersson
left a comment
andersson
left a comment
There was a problem hiding this comment.
None of the three malloc/free patches contains motivation to why we should fix it up. Please include your motivation, so that others know why these changes matters and why they should do the same.
The `part_entry_lba` field in `struct gpt_header` is uint64_t, but the local `lba` variable was declared as `unsigned int`, silently truncating values above UINT_MAX. The buffer `lba_buf[10]` is also too small: a 64-bit value can be up to 20 decimal digits, requiring at least 21 bytes including the null terminator. Combined with an unchecked sprintf(), this is a stack buffer overflow for any LBA value exceeding 9 digits. Fix by: - declaring `lba` as uint64_t to match the source type - increasing lba_buf to 21 bytes - using snprintf() with sizeof to prevent overruns - using PRIu64 for correct format specifier Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
igoropaniuk
force-pushed
the
memleaks_sec_issues
branch
2 times, most recently
from
5f782fc to
4e80cdb
Compare
igoropaniuk
force-pushed
the
memleaks_sec_issues
branch
from
4e80cdb to
cb23b03
Compare
Contributor
Author
Fixed. I also reworked completely the patch addressing the images[] parsing memory leaks, where rather restructuring the parser loop with break/cleanup logic added a fix this at the ownership level. Additionally added one more small fix in a separate commit |
The sector probe buffer in firehose_try_configure() is a small, bounded allocation (max 4096 bytes) used only within this function scope. Replace malloc() with alloca() to: - avoid the need for a free() path and NULL-check error handling - simplify control flow, as the buffer is automatically released on function return The upper bound is compile-time constant (largest entry in sector_sizes[]), well within safe stack allocation limits. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
malloc() can return NULL on allocation failure. Without a check, the subsequent read(fd, ptr, len) dereferences a NULL pointer, causing undefined behavior (typically a segfault). Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
The `len` parameter is size_t (unsigned) while `actual` from libusb_bulk_transfer() is int (signed). Their direct comparison on line 331 triggers -Wsign-compare: if `actual` were somehow negative, the implicit conversion to size_t would produce a large positive value, making the comparison silently wrong. Fixes: b4030fa ("usb: Fix checkpatch warning about unnecessary parenthesis") Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
igoropaniuk
force-pushed
the
memleaks_sec_issues
branch
from
cb23b03 to
74fe0a0
Compare
Contributor
Author
|
Friendly ping on this PR. |
andersson
reviewed
Feb 19, 2026
Collaborator
andersson
left a comment
andersson
left a comment
There was a problem hiding this comment.
Thanks for the update. Unfortunately I recently changed the model for how the archive is managed (no longer assigned to images[0] automatically).
igoropaniuk
force-pushed
the
memleaks_sec_issues
branch
from
74fe0a0 to
5c131bc
Compare
Contributor
Author
|
@andersson both comments are addressed. Thanks! |
On error, decode_programmer_archive() and decode_sahara_config() leak both the input blob and any images partially loaded before the failure. Fix by extending their error paths to call sahara_images_free() and free the blob, mirroring the cleanup already done on success. Also introduce sahara_images_free() to consolidate teardown, drop the misleading 'const' from sahara_image.name, and release sahara_images in qdl_flash() on all exit paths. Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
igoropaniuk
force-pushed
the
memleaks_sec_issues
branch
from
5c131bc to
d192ab7
Compare
andersson
approved these changes
Feb 25, 2026
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
3 participants
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.
No description provided.