| Message ID | 1554098012-105033-1-git-send-email-austin.phillips@planetinnovation.com.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | handlers: Improve archive_handler error handling | expand |
Hi Austin, On 01/04/19 07:53, 'Austin Phillips' via swupdate wrote: > This change makes a number of changes to the archive handler to ensure > that correct cleanup of resources occurs both during normal operation > and in error scenarios. > > Remove memory leaks by freeing memory allocated during archive copy. > Modify control flow so that exit from copy and extract functions > always go through an unwind stage to free any memory and resource > allocations and close all opened handles. > > Adjust archive FIFO creation permissions from 0666 to 0660 to improve > system security during archive extraction and prevent modification of > extraction process by other parts of the system that don't have FIFO > file access. Then, should not we set it to 0600 ? > > Signed-off-by: Austin Phillips <austin.phillips@planetinnovation.com.au> > --- > handlers/archive_handler.c | 142 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 101 insertions(+), 41 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index a44819d..cbac0fa 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -67,16 +67,25 @@ copy_data(struct archive *ar, struct archive *aw) > static void * > extract(void *p) > { > - struct archive *a; > - struct archive *ext; > - struct archive_entry *entry; > + struct archive *a = NULL; > + struct archive *ext = NULL; These generates a warning because the value NULL is overwritten. It is not required to initialize it. > + struct archive_entry *entry = NULL; > int r; > int flags; > struct extract_data *data = (struct extract_data *)p; > flags = data->flags; > + int exitval = -EFAULT; > > a = archive_read_new(); > + if (a == NULL) { if (!a) > + goto out; > + } > + > ext = archive_write_disk_new(); > + if (ext == NULL) { if (!ext) > + goto out; > + } > + > archive_write_disk_set_options(ext, flags); > /* > * Note: archive_write_disk_set_standard_lookup() is useful > @@ -97,7 +106,7 @@ extract(void *p) > if ((r = archive_read_open_filename(a, FIFO, 4096))) { > ERROR("archive_read_open_filename(): %s %d", > archive_error_string(a), r); > - pthread_exit((void *)-1); > + goto out; > } > for (;;) { > r = archive_read_next_header(a, &entry); > @@ -106,7 +115,7 @@ extract(void *p) > if (r != ARCHIVE_OK) { > ERROR("archive_read_next_header(): %s %d", > archive_error_string(a), 1); > - pthread_exit((void *)-1); > + goto out; > } > > if (debug) > @@ -122,28 +131,47 @@ extract(void *p) > if (r != ARCHIVE_OK) { > ERROR("archive_write_finish_entry(): %s", > archive_error_string(ext)); > - pthread_exit((void *)-1); > + goto out; > } > } > > } > > - archive_read_close(a); > - archive_read_free(a); > - pthread_exit((void *)0); > + exitval = 0; > + > +out: > + if (ext != NULL) { if (ext) > + r = archive_write_free(ext); > + if (r) { > + ERROR("archive_write_free(): %s %d", > + archive_error_string(a), r); > + exitval = -EFAULT; > + } Then archive_read_close() and archive_read_free() are not called. Swap the order: if (a) { archive_read_close(a); archive_read_free(a); } if (ext) [ .... > + } > + > + if (a != NULL) { > + archive_read_close(a); > + archive_read_free(a); > + } > + > + pthread_exit((void *)exitval); > } > > static int install_archive_image(struct img_type *img, > void __attribute__ ((__unused__)) *data) > { > char path[255]; > - int fdout; > - int ret = 0; > - char pwd[256]; > + int fdout = -1; Why do we need to initalize it explicitely ? fdout is set to the return value of open. > + int ret = -1; > + int thread_ret = -1; > + char pwd[256] = "\0"; > struct extract_data tf; > pthread_attr_t attr; > void *status; > int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0; > + int is_mounted = 0; > + int exitval = -EFAULT; > + char* FIFO = NULL; > > char* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); > sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); > @@ -153,7 +181,7 @@ static int install_archive_image(struct img_type *img, > > if (strlen(img->path) == 0) { > TRACE("Missing path attribute"); > - return -1; > + goto out; > } Why ? Nothing is allocated, no need to free something, it is more readable if the function exits with failure. > > if (use_mount) { > @@ -161,31 +189,37 @@ static int install_archive_image(struct img_type *img, > if (ret) { > ERROR("Device %s with filesystem %s cannot be mounted", > img->device, img->filesystem); > - return -1; > + goto out; > } Same here. > > + is_mounted = 1; > + > if (snprintf(path, sizeof(path), "%s%s", > DATADST_DIR, img->path) >= (int)sizeof(path)) { > ERROR("Path too long: %s%s", DATADST_DIR, img->path); > - return -1; > + goto out; > } > } else { > if (snprintf(path, sizeof(path), "%s", img->path) >= (int)sizeof(path)) { > ERROR("Path too long: %s", img->path); > - return -1; > + goto out; > } > } > > - char* FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); > + FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); We had already this discussion if the allocation on the stack should be done at the beginning of the function. It is allowed in SWUpdate to allocate when needed. Do not change if there are not other reason (that means, things to be fixed). > sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > unlink(FIFO); > - ret = mkfifo(FIFO, 0666); > + ret = mkfifo(FIFO, 0660); Then, 0600 ? This is an internal FIFO. > if (ret) { > ERROR("FIFO cannot be created in archive handler"); > - return -1; > + goto out; > + } > + > + if (!getcwd(pwd, sizeof(pwd))) { > + ERROR("Failed to determine current working directory"); > + pwd[0] = '\0'; This change the behavior. > + goto out; > } > - if (!getcwd(pwd, sizeof(pwd))) > - return -1; > > /* > * Change to directory where tarball must be extracted > @@ -193,7 +227,7 @@ static int install_archive_image(struct img_type *img, > ret = chdir(path); > if (ret) { > ERROR("Fault: chdir not possible"); > - return -EFAULT; > + goto out; > } > > TRACE("Installing file %s on %s, %s attributes", > @@ -208,42 +242,68 @@ static int install_archive_image(struct img_type *img, > ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; > } > > - ret = pthread_create(&extract_thread, &attr, extract, &tf); > - if (ret) { > + thread_ret = pthread_create(&extract_thread, &attr, extract, &tf); > + if (thread_ret) { > ERROR("Code from pthread_create() is %d", > - ret); > - return -EFAULT; > + thread_ret); > + goto out; > } > > fdout = open(FIFO, O_WRONLY); > + if (fdout < 0) { > + ERROR("Failed to open FIFO %s", FIFO); > + goto out; > + } > > ret = copyimage(&fdout, img, NULL); > - if (ret< 0) { > + if (ret < 0) { > ERROR("Error copying extracted file"); > - return -EFAULT; > + goto out; > } > > - close(fdout); > + exitval = 0; > > - ret = pthread_join(extract_thread, &status); > - if (ret) { > - ERROR("return code from pthread_join() is %d", ret); > - return -EFAULT; > +out: > + if (fdout >= 0) { > + ret = close(fdout); > + if (ret) { > + ERROR("failed to close FIFO %s", FIFO); > + exitval = -EFAULT; > + } > } > > - unlink(FIFO); > - > - ret = chdir(pwd); > + if (thread_ret == 0) { > + ret = pthread_join(extract_thread, &status); > + if (ret) { > + ERROR("return code from pthread_join() is %d", ret); > + exitval = -EFAULT; > + } > + else if ((int)status != 0) { > + ERROR("copyimage status code is %d", (int)status); > + exitval = -EFAULT; > + } > + } > > - if (ret) { > - TRACE("Fault: chdir not possible"); > + if (pwd[0] != '\0') { > + ret = chdir(pwd); > + if (ret) { > + ERROR("chdir failed to revert to directory %s", pwd); > + exitval = -EFAULT; > + } > } > > - if (use_mount) { > - swupdate_umount(DATADST_DIR); > + if (FIFO != NULL) > + unlink(FIFO); > + > + if (is_mounted) { > + ret = swupdate_umount(DATADST_DIR); > + if (ret) { In this case, a failing unmount does not mean that the install fails and it was silently ignored. This change the behavior. > + ERROR("Failed to unmount directory %s", DATADST_DIR); > + exitval = -EFAULT; > + } > } > > - return ret; > + return exitval; > } > > __attribute__((constructor)) > Best regards, Stefano Babic
Hi Stefano, Thanks for reviewing. On Monday, 1 April 2019 20:24:29 UTC+11, Stefano Babic wrote: > Hi Austin, > > On 01/04/19 07:53, 'Austin Phillips' via swupdate wrote: > > This change makes a number of changes to the archive handler to ensure > > that correct cleanup of resources occurs both during normal operation > > and in error scenarios. > > > > Remove memory leaks by freeing memory allocated during archive copy. > > Modify control flow so that exit from copy and extract functions > > always go through an unwind stage to free any memory and resource > > allocations and close all opened handles. > > > > Adjust archive FIFO creation permissions from 0666 to 0660 to improve > > system security during archive extraction and prevent modification of > > extraction process by other parts of the system that don't have FIFO > > file access. > > Then, should not we set it to 0600 ? My reasoning was that it's probably ok for swupdate to run with a different gid than uid, so 0660 is likely a reasonable compromise to not be world readable. But, since this file is used entirely for internal purposes, I'll change to 0600. > > > > > Signed-off-by: Austin Phillips <austin.phillips@planetinnovation.com.au> > > --- > > handlers/archive_handler.c | 142 ++++++++++++++++++++++++++++++++------------- > > 1 file changed, 101 insertions(+), 41 deletions(-) > > > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > > index a44819d..cbac0fa 100644 > > --- a/handlers/archive_handler.c > > +++ b/handlers/archive_handler.c > > @@ -67,16 +67,25 @@ copy_data(struct archive *ar, struct archive *aw) > > static void * > > extract(void *p) > > { > > - struct archive *a; > > - struct archive *ext; > > - struct archive_entry *entry; > > + struct archive *a = NULL; > > + struct archive *ext = NULL; > > These generates a warning because the value NULL is overwritten. It is > not required to initialize it. This is just good practice, the compiler shouldn't warn and the assignments will be optimised away if unused. In this particular case, the only variable which doesn't need initialisation is 'a' so I'll remove its initialiser. The initial value for the other variables is used in the out: block to check if allocated resources need to be freed. > > > + struct archive_entry *entry = NULL; > > int r; > > int flags; > > struct extract_data *data = (struct extract_data *)p; > > flags = data->flags; > > + int exitval = -EFAULT; > > > > a = archive_read_new(); > > + if (a == NULL) { > > if (!a) Will change null checks to your preferred style. > > > + goto out; > > + } > > + > > ext = archive_write_disk_new(); > > + if (ext == NULL) { > > if (!ext) > > > + goto out; > > + } > > + > > archive_write_disk_set_options(ext, flags); > > /* > > * Note: archive_write_disk_set_standard_lookup() is useful > > @@ -97,7 +106,7 @@ extract(void *p) > > if ((r = archive_read_open_filename(a, FIFO, 4096))) { > > ERROR("archive_read_open_filename(): %s %d", > > archive_error_string(a), r); > > - pthread_exit((void *)-1); > > + goto out; > > } > > for (;;) { > > r = archive_read_next_header(a, &entry); > > @@ -106,7 +115,7 @@ extract(void *p) > > if (r != ARCHIVE_OK) { > > ERROR("archive_read_next_header(): %s %d", > > archive_error_string(a), 1); > > - pthread_exit((void *)-1); > > + goto out; > > } > > > > if (debug) > > @@ -122,28 +131,47 @@ extract(void *p) > > if (r != ARCHIVE_OK) { > > ERROR("archive_write_finish_entry(): %s", > > archive_error_string(ext)); > > - pthread_exit((void *)-1); > > + goto out; > > } > > } > > > > } > > > > - archive_read_close(a); > > - archive_read_free(a); > > - pthread_exit((void *)0); > > + exitval = 0; > > + > > +out: > > + if (ext != NULL) { > > if (ext) > > > + r = archive_write_free(ext); > > + if (r) { > > + ERROR("archive_write_free(): %s %d", > > + archive_error_string(a), r); > > + exitval = -EFAULT; > > + } > > Then archive_read_close() and archive_read_free() are not called. Swap > the order: > > if (a) { > archive_read_close(a); > archive_read_free(a); > } > if (ext) [ > .... > No, resources should be deallocated in the reverse order that they were allocated. In this case, setting exitval = -EFAULT from a failed archive_write_free() doesn't stop archive_read_close() and archive_read_free() from being called, it just sets the exitval so that an error will be reported. Calling archive_write_free() and checking its return value is important because it may flush internally cached information to its output. > > + } > > + > > + if (a != NULL) { > > + archive_read_close(a); > > + archive_read_free(a); > > + } > > + > > + pthread_exit((void *)exitval); > > } > > > > static int install_archive_image(struct img_type *img, > > void __attribute__ ((__unused__)) *data) > > { > > char path[255]; > > - int fdout; > > - int ret = 0; > > - char pwd[256]; > > + int fdout = -1; > > Why do we need to initalize it explicitely ? fdout is set to the return > value of open. Because fdout is checked in the out: block to determine if resources should be deallocated. If fdout isn't initialised and there's a jump to out: before the file is opened, fdout will be set to an invalid value during the check. I want to make sure that state variables are always set to a valid value, not some potentially random value which may be on the stack at the start of the function call. > > > + int ret = -1; > > + int thread_ret = -1; > > + char pwd[256] = "\0"; > > struct extract_data tf; > > pthread_attr_t attr; > > void *status; > > int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0; > > + int is_mounted = 0; > > + int exitval = -EFAULT; > > + char* FIFO = NULL; > > > > char* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); > > sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); > > @@ -153,7 +181,7 @@ static int install_archive_image(struct img_type *img, > > > > if (strlen(img->path) == 0) { > > TRACE("Missing path attribute"); > > - return -1; > > + goto out; > > } > > Why ? Nothing is allocated, no need to free something, it is more > readable if the function exits with failure. > As has been shown by the patch, early returns have contributed to a large number of resource leaks. I counted at least 6 possible exit paths causing leaks of resources in this function alone including the file system mount, the current directory, a pthread, creation of the FIFO and an open FIFO file descriptor. That in itself is a good enough reason to change the style of the function to help ensure leaks don't happen. I'll move all non-leaking resource allocations (such as alloca calls) to the start of the function along with early return checks which can occur before any resources are allocated. > > > > if (use_mount) { > > @@ -161,31 +189,37 @@ static int install_archive_image(struct img_type *img, > > if (ret) { > > ERROR("Device %s with filesystem %s cannot be mounted", > > img->device, img->filesystem); > > - return -1; > > + goto out; > > } > > Same here. > > > > > + is_mounted = 1; > > + > > if (snprintf(path, sizeof(path), "%s%s", > > DATADST_DIR, img->path) >= (int)sizeof(path)) { > > ERROR("Path too long: %s%s", DATADST_DIR, img->path); > > - return -1; > > + goto out; > > } > > } else { > > if (snprintf(path, sizeof(path), "%s", img->path) >= (int)sizeof(path)) { > > ERROR("Path too long: %s", img->path); > > - return -1; > > + goto out; > > } > > } > > > > - char* FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); > > + FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); > > We had already this discussion if the allocation on the stack should be > done at the beginning of the function. It is allowed in SWUpdate to > allocate when needed. Do not change if there are not other reason (that > means, things to be fixed). The allocation for FIFO hasn't moved. See above, I'll move to top. The FIFO needs to be unlinked during cleanup so it's important that FIFO contains a valid name before any jump to out. > > > sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > > unlink(FIFO); > > - ret = mkfifo(FIFO, 0666); > > + ret = mkfifo(FIFO, 0660); > > Then, 0600 ? This is an internal FIFO. Will change. > > > if (ret) { > > ERROR("FIFO cannot be created in archive handler"); > > - return -1; > > + goto out; > > + } > > + > > + if (!getcwd(pwd, sizeof(pwd))) { > > + ERROR("Failed to determine current working directory"); > > + pwd[0] = '\0'; > > This change the behavior. Previously this was an early silent return which leaked resources and is one of the issues the patch fixes. Setting pwd[0] = '\0' indicates to the cleanup code that it doesn't need to do a chdir() on cleanup. > > > + goto out; > > } > > - if (!getcwd(pwd, sizeof(pwd))) > > - return -1; > > > > /* > > * Change to directory where tarball must be extracted > > @@ -193,7 +227,7 @@ static int install_archive_image(struct img_type *img, > > ret = chdir(path); > > if (ret) { > > ERROR("Fault: chdir not possible"); > > - return -EFAULT; > > + goto out; > > } > > > > TRACE("Installing file %s on %s, %s attributes", > > @@ -208,42 +242,68 @@ static int install_archive_image(struct img_type *img, > > ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; > > } > > > > - ret = pthread_create(&extract_thread, &attr, extract, &tf); > > - if (ret) { > > + thread_ret = pthread_create(&extract_thread, &attr, extract, &tf); > > + if (thread_ret) { > > ERROR("Code from pthread_create() is %d", > > - ret); > > - return -EFAULT; > > + thread_ret); > > + goto out; > > } > > > > fdout = open(FIFO, O_WRONLY); > > + if (fdout < 0) { > > + ERROR("Failed to open FIFO %s", FIFO); > > + goto out; > > + } > > > > ret = copyimage(&fdout, img, NULL); > > - if (ret< 0) { > > + if (ret < 0) { > > ERROR("Error copying extracted file"); > > - return -EFAULT; > > + goto out; > > } > > > > - close(fdout); > > + exitval = 0; > > > > - ret = pthread_join(extract_thread, &status); > > - if (ret) { > > - ERROR("return code from pthread_join() is %d", ret); > > - return -EFAULT; > > +out: > > + if (fdout >= 0) { > > + ret = close(fdout); > > + if (ret) { > > + ERROR("failed to close FIFO %s", FIFO); > > + exitval = -EFAULT; > > + } > > } > > > > - unlink(FIFO); > > - > > - ret = chdir(pwd); > > + if (thread_ret == 0) { > > + ret = pthread_join(extract_thread, &status); > > + if (ret) { > > + ERROR("return code from pthread_join() is %d", ret); > > + exitval = -EFAULT; > > + } > > + else if ((int)status != 0) { > > + ERROR("copyimage status code is %d", (int)status); > > + exitval = -EFAULT; > > + } > > + } > > > > - if (ret) { > > - TRACE("Fault: chdir not possible"); > > + if (pwd[0] != '\0') { > > + ret = chdir(pwd); > > + if (ret) { > > + ERROR("chdir failed to revert to directory %s", pwd); > > + exitval = -EFAULT; > > + } > > } > > > > - if (use_mount) { > > - swupdate_umount(DATADST_DIR); > > + if (FIFO != NULL) > > + unlink(FIFO); > > + > > + if (is_mounted) { > > + ret = swupdate_umount(DATADST_DIR); > > + if (ret) { > > In this case, a failing unmount does not mean that the install fails and > it was silently ignored. This change the behavior. Silently ignoring errors isn't ideal. I'll leave the message of failure to umount but not return an error to the caller. Existing behaviour on exit was to only report a failure to the caller for a pthread_join error, all other failures were silently ignored and I'm not sure why this has been chosen. Failure of any of these cleanups is really an error and should be reported as such. I'll revert to existing behaviour but I think its not desirable behaviour to report success to the caller for any of these errors. > > > + ERROR("Failed to unmount directory %s", DATADST_DIR); > > + exitval = -EFAULT; > > + } > > } > > > > - return ret; > > + return exitval; > > } > > > > __attribute__((constructor)) > > > > Best regards, > Stefano Babic I'll submit a V2 patch. Regards Austin
diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..cbac0fa 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -67,16 +67,25 @@ copy_data(struct archive *ar, struct archive *aw) static void * extract(void *p) { - struct archive *a; - struct archive *ext; - struct archive_entry *entry; + struct archive *a = NULL; + struct archive *ext = NULL; + struct archive_entry *entry = NULL; int r; int flags; struct extract_data *data = (struct extract_data *)p; flags = data->flags; + int exitval = -EFAULT; a = archive_read_new(); + if (a == NULL) { + goto out; + } + ext = archive_write_disk_new(); + if (ext == NULL) { + goto out; + } + archive_write_disk_set_options(ext, flags); /* * Note: archive_write_disk_set_standard_lookup() is useful @@ -97,7 +106,7 @@ extract(void *p) if ((r = archive_read_open_filename(a, FIFO, 4096))) { ERROR("archive_read_open_filename(): %s %d", archive_error_string(a), r); - pthread_exit((void *)-1); + goto out; } for (;;) { r = archive_read_next_header(a, &entry); @@ -106,7 +115,7 @@ extract(void *p) if (r != ARCHIVE_OK) { ERROR("archive_read_next_header(): %s %d", archive_error_string(a), 1); - pthread_exit((void *)-1); + goto out; } if (debug) @@ -122,28 +131,47 @@ extract(void *p) if (r != ARCHIVE_OK) { ERROR("archive_write_finish_entry(): %s", archive_error_string(ext)); - pthread_exit((void *)-1); + goto out; } } } - archive_read_close(a); - archive_read_free(a); - pthread_exit((void *)0); + exitval = 0; + +out: + if (ext != NULL) { + r = archive_write_free(ext); + if (r) { + ERROR("archive_write_free(): %s %d", + archive_error_string(a), r); + exitval = -EFAULT; + } + } + + if (a != NULL) { + archive_read_close(a); + archive_read_free(a); + } + + pthread_exit((void *)exitval); } static int install_archive_image(struct img_type *img, void __attribute__ ((__unused__)) *data) { char path[255]; - int fdout; - int ret = 0; - char pwd[256]; + int fdout = -1; + int ret = -1; + int thread_ret = -1; + char pwd[256] = "\0"; struct extract_data tf; pthread_attr_t attr; void *status; int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0; + int is_mounted = 0; + int exitval = -EFAULT; + char* FIFO = NULL; char* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); @@ -153,7 +181,7 @@ static int install_archive_image(struct img_type *img, if (strlen(img->path) == 0) { TRACE("Missing path attribute"); - return -1; + goto out; } if (use_mount) { @@ -161,31 +189,37 @@ static int install_archive_image(struct img_type *img, if (ret) { ERROR("Device %s with filesystem %s cannot be mounted", img->device, img->filesystem); - return -1; + goto out; } + is_mounted = 1; + if (snprintf(path, sizeof(path), "%s%s", DATADST_DIR, img->path) >= (int)sizeof(path)) { ERROR("Path too long: %s%s", DATADST_DIR, img->path); - return -1; + goto out; } } else { if (snprintf(path, sizeof(path), "%s", img->path) >= (int)sizeof(path)) { ERROR("Path too long: %s", img->path); - return -1; + goto out; } } - char* FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); + FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); unlink(FIFO); - ret = mkfifo(FIFO, 0666); + ret = mkfifo(FIFO, 0660); if (ret) { ERROR("FIFO cannot be created in archive handler"); - return -1; + goto out; + } + + if (!getcwd(pwd, sizeof(pwd))) { + ERROR("Failed to determine current working directory"); + pwd[0] = '\0'; + goto out; } - if (!getcwd(pwd, sizeof(pwd))) - return -1; /* * Change to directory where tarball must be extracted @@ -193,7 +227,7 @@ static int install_archive_image(struct img_type *img, ret = chdir(path); if (ret) { ERROR("Fault: chdir not possible"); - return -EFAULT; + goto out; } TRACE("Installing file %s on %s, %s attributes", @@ -208,42 +242,68 @@ static int install_archive_image(struct img_type *img, ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; } - ret = pthread_create(&extract_thread, &attr, extract, &tf); - if (ret) { + thread_ret = pthread_create(&extract_thread, &attr, extract, &tf); + if (thread_ret) { ERROR("Code from pthread_create() is %d", - ret); - return -EFAULT; + thread_ret); + goto out; } fdout = open(FIFO, O_WRONLY); + if (fdout < 0) { + ERROR("Failed to open FIFO %s", FIFO); + goto out; + } ret = copyimage(&fdout, img, NULL); - if (ret< 0) { + if (ret < 0) { ERROR("Error copying extracted file"); - return -EFAULT; + goto out; } - close(fdout); + exitval = 0; - ret = pthread_join(extract_thread, &status); - if (ret) { - ERROR("return code from pthread_join() is %d", ret); - return -EFAULT; +out: + if (fdout >= 0) { + ret = close(fdout); + if (ret) { + ERROR("failed to close FIFO %s", FIFO); + exitval = -EFAULT; + } } - unlink(FIFO); - - ret = chdir(pwd); + if (thread_ret == 0) { + ret = pthread_join(extract_thread, &status); + if (ret) { + ERROR("return code from pthread_join() is %d", ret); + exitval = -EFAULT; + } + else if ((int)status != 0) { + ERROR("copyimage status code is %d", (int)status); + exitval = -EFAULT; + } + } - if (ret) { - TRACE("Fault: chdir not possible"); + if (pwd[0] != '\0') { + ret = chdir(pwd); + if (ret) { + ERROR("chdir failed to revert to directory %s", pwd); + exitval = -EFAULT; + } } - if (use_mount) { - swupdate_umount(DATADST_DIR); + if (FIFO != NULL) + unlink(FIFO); + + if (is_mounted) { + ret = swupdate_umount(DATADST_DIR); + if (ret) { + ERROR("Failed to unmount directory %s", DATADST_DIR); + exitval = -EFAULT; + } } - return ret; + return exitval; } __attribute__((constructor))
This change makes a number of changes to the archive handler to ensure that correct cleanup of resources occurs both during normal operation and in error scenarios. Remove memory leaks by freeing memory allocated during archive copy. Modify control flow so that exit from copy and extract functions always go through an unwind stage to free any memory and resource allocations and close all opened handles. Adjust archive FIFO creation permissions from 0666 to 0660 to improve system security during archive extraction and prevent modification of extraction process by other parts of the system that don't have FIFO file access. Signed-off-by: Austin Phillips <austin.phillips@planetinnovation.com.au> --- handlers/archive_handler.c | 142 ++++++++++++++++++++++++++++++++------------- 1 file changed, 101 insertions(+), 41 deletions(-)