| Message ID | 1556074645-37825-1-git-send-email-austin.phillips@planetinnovation.com.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] handlers: Improve archive_handler error handling | expand |
Hi Austin, On 24/04/19 04:57, '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 0600 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 | 138 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 99 insertions(+), 39 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index a44819d..7f12761 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -35,6 +35,7 @@ pthread_t extract_thread; > > struct extract_data { > int flags; > + int exitval; > }; > > static int > @@ -68,15 +69,24 @@ static void * > extract(void *p) > { > struct archive *a; > - struct archive *ext; > - struct archive_entry *entry; > + 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) { > + goto out; > + } > + > ext = archive_write_disk_new(); > + if (!ext) { > + goto out; > + } > + > archive_write_disk_set_options(ext, flags); > /* > * Note: archive_write_disk_set_standard_lookup() is useful > @@ -97,7 +107,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 +116,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,32 +132,53 @@ 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) { > + r = archive_write_free(ext); > + if (r) { > + ERROR("archive_write_free(): %s %d", > + archive_error_string(a), r); > + exitval = -EFAULT; > + } > + } > + > + if (a) { > + archive_read_close(a); > + archive_read_free(a); > + } > + > + data->exitval = exitval; > + pthread_exit(NULL); > } > > 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* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); > sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); > > + char * FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); > + sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > + As you are fixing this : it should be checked that alloca() return a valid pointer, that is DATADST_DIR and FIFO must be not Null. > pthread_attr_init(&attr); > pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); > > @@ -164,28 +195,32 @@ static int install_archive_image(struct img_type *img, > return -1; > } > > + 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); > - sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > unlink(FIFO); > - ret = mkfifo(FIFO, 0666); > + ret = mkfifo(FIFO, 0600); > 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 +228,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", > @@ -201,6 +236,7 @@ static int install_archive_image(struct img_type *img, > img->preserve_attributes ? "preserving" : "ignoring"); > > tf.flags = 0; > + tf.exitval = -EFAULT; > > if (img->preserve_attributes) { > tf.flags |= ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | > @@ -208,42 +244,66 @@ 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); > + } > } > > - unlink(FIFO); > + if (!thread_ret) { > + void *status; But you have already checked thread_ret before. At this point, thread_ret is surely 0. > > - ret = chdir(pwd); > + ret = pthread_join(extract_thread, &status); > + if (ret) { > + ERROR("return code from pthread_join() is %d", ret); > + exitval = -EFAULT; > + } > + else if (tf.exitval != 0) { > + ERROR("copyimage status code is %d", tf.exitval); > + exitval = -EFAULT; > + } > + } > > - if (ret) { > - TRACE("Fault: chdir not possible"); > + if (pwd[0]) { > + ret = chdir(pwd); > + if (ret) { > + ERROR("chdir failed to revert to directory %s", pwd); > + } > } > > - if (use_mount) { > - swupdate_umount(DATADST_DIR); > + unlink(FIFO); > + > + if (is_mounted) { > + ret = swupdate_umount(DATADST_DIR); > + if (ret) { > + TRACE("Failed to unmount directory %s", DATADST_DIR); > + } > } > > - return ret; > + return exitval; > } > > __attribute__((constructor)) > Best regards, Stefano Babic
Hi Stefano, > Hi Austin, > > On 24/04/19 04:57, '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 0600 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 | 138 > > ++++++++++++++++++++++++++++++++------------- > > 1 file changed, 99 insertions(+), 39 deletions(-) > > > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > > index a44819d..7f12761 100644 > > --- a/handlers/archive_handler.c > > +++ b/handlers/archive_handler.c > > @@ -35,6 +35,7 @@ pthread_t extract_thread; > > > > struct extract_data { > > int flags; > > + int exitval; > > }; > > > > static int > > @@ -68,15 +69,24 @@ static void * > > extract(void *p) > > { > > struct archive *a; > > - struct archive *ext; > > - struct archive_entry *entry; > > + 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) { > > + goto out; > > + } > > + > > ext = archive_write_disk_new(); > > + if (!ext) { > > + goto out; > > + } > > + > > archive_write_disk_set_options(ext, flags); > > /* > > * Note: archive_write_disk_set_standard_lookup() is useful @@ - > 97,7 > > +107,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 +116,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,32 +132,53 @@ 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) { > > + r = archive_write_free(ext); > > + if (r) { > > + ERROR("archive_write_free(): %s %d", > > + archive_error_string(a), r); > > + exitval = -EFAULT; > > + } > > + } > > + > > + if (a) { > > + archive_read_close(a); > > + archive_read_free(a); > > + } > > + > > + data->exitval = exitval; > > + pthread_exit(NULL); > > } > > > > 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* DATADST_DIR = > alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); > > sprintf(DATADST_DIR, "%s%s", get_tmpdir(), > DATADST_DIR_SUFFIX); > > > > + char * FIFO = > alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); > > + sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > > + > > As you are fixing this : it should be checked that alloca() return a valid > pointer, > that is DATADST_DIR and FIFO must be not Null. alloca allocates space on the stack. If the stack overflows as a result of alloca failure, program behaviour is undefined. It is not documented that alloca can return a NULL pointer. Since this patch provides a much better cleanup behaviour than previous, it would be a simple change to convert these to malloc calls with associated NULL checks on allocation, and free on exit. I think it's acceptable as is. Do you have a preference for how to proceed? > > > pthread_attr_init(&attr); > > pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); > > > > @@ -164,28 +195,32 @@ static int install_archive_image(struct img_type > *img, > > return -1; > > } > > > > + 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); > > - sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); > > unlink(FIFO); > > - ret = mkfifo(FIFO, 0666); > > + ret = mkfifo(FIFO, 0600); > > 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 > > +228,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", @@ -201,6 +236,7 > @@ > > static int install_archive_image(struct img_type *img, > > img->preserve_attributes ? "preserving" : "ignoring"); > > > > tf.flags = 0; > > + tf.exitval = -EFAULT; > > > > if (img->preserve_attributes) { > > tf.flags |= ARCHIVE_EXTRACT_OWNER | > ARCHIVE_EXTRACT_PERM | @@ > > -208,42 +244,66 @@ 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); > > + } > > } > > > > - unlink(FIFO); > > + if (!thread_ret) { > > + void *status; > > > But you have already checked thread_ret before. At this point, thread_ret > is > surely 0. This code is part of the function exit path. An exit may occur earlier than when the thread is started so a check against thread_ret is certainly needed. In case it isn't clear from the patch, the workflow of the function is like this: - Initialise all resource-holding variables to sentinel values at start of function. e.g. Set file descriptors to negative values, pointers to NULL etc. - Do all regular function behaviour. If resources are allocated, set resource-holding variable to some non-sentinel value. e.g. File descriptors would be set >= 0. On failures, jump to out. - At out:, free all resources in reverse order of creation. This involves checking resource-holding variables. If they contain their original sentinel value set at top of function, there is no resource to free. This is a good pattern for C code which allocates resources that only exist for the lifetime of the function. It requires that variables are initialised before use to known values, also a good practice. It has the benefit that an early exit on failure can occur from deep within the function without the programmer having to keep in mind exactly what resources have been allocated up until the point of failure, keeping the main logic flow simpler. > > > > > - ret = chdir(pwd); > > + ret = pthread_join(extract_thread, &status); > > + if (ret) { > > + ERROR("return code from pthread_join() is %d", ret); > > + exitval = -EFAULT; > > + } > > + else if (tf.exitval != 0) { > > + ERROR("copyimage status code is %d", tf.exitval); > > + exitval = -EFAULT; > > + } > > + } > > > > - if (ret) { > > - TRACE("Fault: chdir not possible"); > > + if (pwd[0]) { > > + ret = chdir(pwd); > > + if (ret) { > > + ERROR("chdir failed to revert to directory %s", pwd); > > + } > > } > > > > - if (use_mount) { > > - swupdate_umount(DATADST_DIR); > > + unlink(FIFO); > > + > > + if (is_mounted) { > > + ret = swupdate_umount(DATADST_DIR); > > + if (ret) { > > + TRACE("Failed to unmount directory %s", > DATADST_DIR); > > + } > > } > > > > - return ret; > > + return exitval; > > } > > > > __attribute__((constructor)) > > > > Best regards, > Stefano Babic > Regards, Austin
Hi Austin, On 26/04/19 01:35, 'Austin Phillips' via swupdate wrote: > Hi Stefano, > >> Hi Austin, >> >> On 24/04/19 04:57, '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 0600 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 | 138 >>> ++++++++++++++++++++++++++++++++------------- >>> 1 file changed, 99 insertions(+), 39 deletions(-) >>> >>> diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c >>> index a44819d..7f12761 100644 >>> --- a/handlers/archive_handler.c >>> +++ b/handlers/archive_handler.c >>> @@ -35,6 +35,7 @@ pthread_t extract_thread; >>> >>> struct extract_data { >>> int flags; >>> + int exitval; >>> }; >>> >>> static int >>> @@ -68,15 +69,24 @@ static void * >>> extract(void *p) >>> { >>> struct archive *a; >>> - struct archive *ext; >>> - struct archive_entry *entry; >>> + 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) { >>> + goto out; >>> + } >>> + >>> ext = archive_write_disk_new(); >>> + if (!ext) { >>> + goto out; >>> + } >>> + >>> archive_write_disk_set_options(ext, flags); >>> /* >>> * Note: archive_write_disk_set_standard_lookup() is useful @@ - >> 97,7 >>> +107,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 +116,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,32 +132,53 @@ 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) { >>> + r = archive_write_free(ext); >>> + if (r) { >>> + ERROR("archive_write_free(): %s %d", >>> + archive_error_string(a), r); >>> + exitval = -EFAULT; >>> + } >>> + } >>> + >>> + if (a) { >>> + archive_read_close(a); >>> + archive_read_free(a); >>> + } >>> + >>> + data->exitval = exitval; >>> + pthread_exit(NULL); >>> } >>> >>> 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* DATADST_DIR = >> alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); >>> sprintf(DATADST_DIR, "%s%s", get_tmpdir(), >> DATADST_DIR_SUFFIX); >>> >>> + char * FIFO = >> alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); >>> + sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); >>> + >> >> As you are fixing this : it should be checked that alloca() return a valid >> pointer, >> that is DATADST_DIR and FIFO must be not Null. > > alloca allocates space on the stack. If the stack overflows as a result of > alloca failure, program behaviour is undefined. Ouch, right. Yes, it is. In case of stack overflow, the process should get a SEGV. It is ok, no changes required. > It is not documented that > alloca can return a NULL pointer. Since this patch provides a much better > cleanup behaviour than previous, it would be a simple change to convert > these to malloc calls with associated NULL checks on allocation, and free on > exit. > > I think it's acceptable as is. Do you have a preference for how to proceed? > >> >>> pthread_attr_init(&attr); >>> pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); >>> >>> @@ -164,28 +195,32 @@ static int install_archive_image(struct img_type >> *img, >>> return -1; >>> } >>> >>> + 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); >>> - sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); >>> unlink(FIFO); >>> - ret = mkfifo(FIFO, 0666); >>> + ret = mkfifo(FIFO, 0600); >>> 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 >>> +228,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", @@ -201,6 +236,7 >> @@ >>> static int install_archive_image(struct img_type *img, >>> img->preserve_attributes ? "preserving" : "ignoring"); >>> >>> tf.flags = 0; >>> + tf.exitval = -EFAULT; >>> >>> if (img->preserve_attributes) { >>> tf.flags |= ARCHIVE_EXTRACT_OWNER | >> ARCHIVE_EXTRACT_PERM | @@ >>> -208,42 +244,66 @@ 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); >>> + } >>> } >>> >>> - unlink(FIFO); >>> + if (!thread_ret) { >>> + void *status; >> >> >> But you have already checked thread_ret before. At this point, thread_ret >> is >> surely 0. > > This code is part of the function exit path. An exit may occur earlier than > when the thread is started so a check against thread_ret is certainly > needed. I have misunderstood when the check was done and I was thought it was in the main branch instead of exit path. Got it, it is ok. > > In case it isn't clear from the patch, the workflow of the function is like > this: > - Initialise all resource-holding variables to sentinel values at start of > function. e.g. Set file descriptors to negative values, pointers to NULL > etc. > - Do all regular function behaviour. If resources are allocated, set > resource-holding variable to some non-sentinel value. e.g. File descriptors > would be set >= 0. On failures, jump to out. > - At out:, free all resources in reverse order of creation. This involves > checking resource-holding variables. If they contain their original > sentinel value set at top of function, there is no resource to free. > Code seems self explaining after applying the patch. > This is a good pattern for C code which allocates resources that only exist > for the lifetime of the function. It requires that variables are > initialised before use to known values, also a good practice. It has the > benefit that an early exit on failure can occur from deep within the > function without the programmer having to keep in mind exactly what > resources have been allocated up until the point of failure, keeping the > main logic flow simpler. That's ok - I apply it. Best regards, Stefano Babic > >> >>> >>> - ret = chdir(pwd); >>> + ret = pthread_join(extract_thread, &status); >>> + if (ret) { >>> + ERROR("return code from pthread_join() is %d", ret); >>> + exitval = -EFAULT; >>> + } >>> + else if (tf.exitval != 0) { >>> + ERROR("copyimage status code is %d", tf.exitval); >>> + exitval = -EFAULT; >>> + } >>> + } >>> >>> - if (ret) { >>> - TRACE("Fault: chdir not possible"); >>> + if (pwd[0]) { >>> + ret = chdir(pwd); >>> + if (ret) { >>> + ERROR("chdir failed to revert to directory %s", pwd); >>> + } >>> } >>> >>> - if (use_mount) { >>> - swupdate_umount(DATADST_DIR); >>> + unlink(FIFO); >>> + >>> + if (is_mounted) { >>> + ret = swupdate_umount(DATADST_DIR); >>> + if (ret) { >>> + TRACE("Failed to unmount directory %s", >> DATADST_DIR); >>> + } >>> } >>> >>> - return ret; >>> + return exitval; >>> } >>> >>> __attribute__((constructor)) >>> >> >> Best regards, >> Stefano Babic >> > > Regards, > Austin >
diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..7f12761 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -35,6 +35,7 @@ pthread_t extract_thread; struct extract_data { int flags; + int exitval; }; static int @@ -68,15 +69,24 @@ static void * extract(void *p) { struct archive *a; - struct archive *ext; - struct archive_entry *entry; + 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) { + goto out; + } + ext = archive_write_disk_new(); + if (!ext) { + goto out; + } + archive_write_disk_set_options(ext, flags); /* * Note: archive_write_disk_set_standard_lookup() is useful @@ -97,7 +107,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 +116,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,32 +132,53 @@ 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) { + r = archive_write_free(ext); + if (r) { + ERROR("archive_write_free(): %s %d", + archive_error_string(a), r); + exitval = -EFAULT; + } + } + + if (a) { + archive_read_close(a); + archive_read_free(a); + } + + data->exitval = exitval; + pthread_exit(NULL); } 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* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); + char * FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); + sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); + pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); @@ -164,28 +195,32 @@ static int install_archive_image(struct img_type *img, return -1; } + 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); - sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); unlink(FIFO); - ret = mkfifo(FIFO, 0666); + ret = mkfifo(FIFO, 0600); 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 +228,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", @@ -201,6 +236,7 @@ static int install_archive_image(struct img_type *img, img->preserve_attributes ? "preserving" : "ignoring"); tf.flags = 0; + tf.exitval = -EFAULT; if (img->preserve_attributes) { tf.flags |= ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | @@ -208,42 +244,66 @@ 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); + } } - unlink(FIFO); + if (!thread_ret) { + void *status; - ret = chdir(pwd); + ret = pthread_join(extract_thread, &status); + if (ret) { + ERROR("return code from pthread_join() is %d", ret); + exitval = -EFAULT; + } + else if (tf.exitval != 0) { + ERROR("copyimage status code is %d", tf.exitval); + exitval = -EFAULT; + } + } - if (ret) { - TRACE("Fault: chdir not possible"); + if (pwd[0]) { + ret = chdir(pwd); + if (ret) { + ERROR("chdir failed to revert to directory %s", pwd); + } } - if (use_mount) { - swupdate_umount(DATADST_DIR); + unlink(FIFO); + + if (is_mounted) { + ret = swupdate_umount(DATADST_DIR); + if (ret) { + TRACE("Failed to unmount directory %s", DATADST_DIR); + } } - 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 0600 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 | 138 ++++++++++++++++++++++++++++++++------------- 1 file changed, 99 insertions(+), 39 deletions(-)