Address memory leaks/mitigate buffer overflows #161
Conversation
The original `part_entry_lba` field in `struct gpt_header` is uint64_t. A 64-bit unsigned integer can be up to 18446744073709551615 -> 20 digits, so lba_buf should be at least 21 bytes(including '\0'). Redefine local `lba` as uint64_t, increase lba_buf size, and use snprintf() instead of sprintf(). Signed-off-by: Igor Opaniuk <[email protected]>
Check malloc() return value for a sector buffer. Signed-off-by: Igor Opaniuk <[email protected]>
Check malloc() return value in load_sahara_image() implementation. Signed-off-by: Igor Opaniuk <[email protected]>
Properly free all allocated resources in decode_programmer_archive() if failure occurs. Signed-off-by: Igor Opaniuk <[email protected]>
|
@lumag @andersson @konradybcio could you take a look please |
| * even if some images aren't initialized, it's safe to enumerate over each | ||
| * image and try to free, as otherwise image[i] will be filled with zeros. | ||
| * | ||
| * first image isn't freed if error occurred |
There was a problem hiding this comment.
The kernel-doc answers this question... making this comment a bit weird.
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.
| } | ||
| } | ||
|
|
||
| free(buf); |
There was a problem hiding this comment.
I think replacing the malloc() with alloca() would be cleaner, then we don't have to explicitly free the memory (and we don't need the NULL check...)
| continue; | ||
|
|
||
| return 1; | ||
| if (images[i].ptr) |
There was a problem hiding this comment.
Calling free(NULL) is valid, so no need for the extra conditionals.
| blob->len = 0; | ||
| /* | ||
| * even if some images aren't initialized, it's safe to enumerate over each | ||
| * image and try to free, as otherwise image[i] will be filled with zeros. |
There was a problem hiding this comment.
If needed, this would better be added to the comment above the function, stating that the images array must be zero-initialized (but I don't think it's necessary to document that).
| * even if some images aren't initialized, it's safe to enumerate over each | ||
| * image and try to free, as otherwise image[i] will be filled with zeros. | ||
| * | ||
| * first image isn't freed if error occurred |
There was a problem hiding this comment.
The kernel-doc answers this question... making this comment a bit weird.
| * first image isn't freed if error occurred | ||
| */ | ||
| for (size_t i = 0; i < MAPPING_SZ; i++) { | ||
| if (ret != 1 && i == 0) |
There was a problem hiding this comment.
Please see if you can write this in a less clever way, or perhaps add the comment here.
| if (images[i].ptr) | ||
| free(images[i].ptr); | ||
| if (images[i].name) | ||
| free((void *)images[i].name); |
There was a problem hiding this comment.
Perhaps drop the const instead of type casting here?
3 participants
No description provided.