-
Notifications
You must be signed in to change notification settings - Fork 112
Address memory leaks/mitigate buffer overflows #161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
eec6e64
6365bf3
3efa6a2
df9dd37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,20 +171,22 @@ static int decode_programmer_archive(struct sahara_image *images) | |
| void *ptr = blob->ptr; | ||
| void *end = blob->ptr + blob->len; | ||
| long id; | ||
| int ret; | ||
|
|
||
| if (blob->len < sizeof(*hdr) || memcmp(ptr, CPIO_MAGIC, 6)) | ||
| return 0; | ||
|
|
||
| ret = -1; | ||
| for (;;) { | ||
| if (ptr + sizeof(*hdr) > end) { | ||
| ux_err("programmer archive is truncated\n"); | ||
| return -1; | ||
| break; | ||
| } | ||
| hdr = ptr; | ||
|
|
||
| if (memcmp(hdr->c_magic, "070701", 6)) { | ||
| ux_err("expected cpio header in programmer archive\n"); | ||
| return -1; | ||
| break; | ||
| } | ||
|
|
||
| filesize = parse_ascii_hex32(hdr->c_filesize); | ||
|
|
@@ -193,44 +195,70 @@ static int decode_programmer_archive(struct sahara_image *images) | |
| ptr += sizeof(*hdr); | ||
| if (ptr + namesize > end || ptr + filesize + namesize > end) { | ||
| ux_err("programmer archive is truncated\n"); | ||
| return -1; | ||
| break; | ||
| } | ||
|
|
||
| if (namesize > sizeof(name)) { | ||
| ux_err("unexpected filename length in progammer archive\n"); | ||
| return -1; | ||
| break; | ||
| } | ||
| memcpy(name, ptr, namesize); | ||
|
|
||
| if (!memcmp(name, "TRAILER!!!", 11)) | ||
| if (!memcmp(name, "TRAILER!!!", 11)) { | ||
| ret = 1; | ||
| break; | ||
| } | ||
|
|
||
| tok = strtok_r(name, ":", &save); | ||
| id = strtoul(tok, NULL, 0); | ||
| if (id == 0 || id >= MAPPING_SZ) { | ||
| ux_err("invalid image id \"%s\" in programmer archive\n", tok); | ||
| return -1; | ||
| break; | ||
| } | ||
|
|
||
| ptr += namesize; | ||
| ptr = ALIGN_UP(ptr, 4); | ||
|
|
||
| tok = strtok_r(NULL, ":", &save); | ||
| if (tok) | ||
| if (tok) { | ||
| images[id].name = strdup(tok); | ||
| if (!images[id].name) { | ||
| ux_err("failed to allocated buffer for an image name\n"); | ||
| break; | ||
| } | ||
| } | ||
| images[id].len = filesize; | ||
| images[id].ptr = malloc(filesize); | ||
| if (!images[id].ptr) { | ||
| ux_err("failed to allocated buffer for an image, len = %z\n", images[id].len); | ||
| break; | ||
| } | ||
| memcpy(images[id].ptr, ptr, filesize); | ||
|
|
||
| ptr += filesize; | ||
| ptr = ALIGN_UP(ptr, 4); | ||
| } | ||
|
|
||
| free(blob->ptr); | ||
| blob->ptr = NULL; | ||
| 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
| * | ||
| * first image isn't freed if error occurred | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The kernel-doc answers this question... making this comment a bit weird. |
||
| */ | ||
| for (size_t i = 0; i < MAPPING_SZ; i++) { | ||
| if (ret != 1 && i == 0) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see if you can write this in a less clever way, or perhaps add the comment here. |
||
| continue; | ||
|
|
||
| return 1; | ||
| if (images[i].ptr) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling free(NULL) is valid, so no need for the extra conditionals. |
||
| free(images[i].ptr); | ||
| if (images[i].name) | ||
| free((void *)images[i].name); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps drop the const instead of type casting here? |
||
| images[i].ptr = NULL; | ||
| images[i].name = NULL; | ||
| images[i].len = 0; | ||
| } | ||
|
|
||
| return ret; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...)