| Message ID | 1554163929-114356-1-git-send-email-austin.phillips@planetinnovation.com.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V2] handlers: Improve archive_handler error handling | expand |
Bump. Are there additional changes required to this patch before it can be merged? Regards Austin On Tuesday, 2 April 2019 11:12:46 UTC+11, Austin Phillips 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 | 134 > ++++++++++++++++++++++++++++++++------------- > 1 file changed, 95 insertions(+), 39 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index a44819d..49c5cad 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -68,15 +68,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 +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,32 +131,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); > + } > + > + 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* 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 +194,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 +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,64 @@ 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); > - > - ret = chdir(pwd); > + if (!thread_ret) { > + 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]) { > + 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)) > -- > 2.7.4 > > > -- > > > > > *IMPORTANT > NOTE. *If you are > NOT AN AUTHORISED RECIPIENT of this > e-mail, please contact Planet Innovation by return e-mail or by telephone > on +61-3-9945 7510. In this case, you > should not read, print, re-transmit, > store or act in reliance on this e-mail or > any attachments, and should > destroy all copies of them. This e-mail and > any attachments are > confidential and may contain legally privileged information > and/or > copyright material. You should only re-transmit, distribute or > commercialise the material if you > are authorised to do so. Although we use > antimalware, we disclaim any liability for any malware in any message or > attachment. This > notice should not be removed. > > ** >
Hi Austin, On 23/04/19 02:08, 'Austin Phillips' via swupdate wrote: > Bump. Are there additional changes required to this patch before it can > be merged? > I left out to check it again, but I do not see any reasons to block it. I pick it up and I merge for incoming release. Best regards, Stefano Babic > Regards > Austin > > On Tuesday, 2 April 2019 11:12:46 UTC+11, Austin Phillips 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 > <mailto:austin.phillips@planetinnovation.com.au>> > --- > handlers/archive_handler.c | 134 > ++++++++++++++++++++++++++++++++------------- > 1 file changed, 95 insertions(+), 39 deletions(-) > > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index a44819d..49c5cad 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -68,15 +68,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 +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,32 +131,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); > + } > + > + 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* 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 +194,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 +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,64 @@ 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); > - > - ret = chdir(pwd); > + if (!thread_ret) { > + 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]) { > + 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)) > -- > 2.7.4 > > > -- > > > > > *IMPORTANT > NOTE. *If you are > NOT AN AUTHORISED RECIPIENT of this > e-mail, please contact Planet Innovation by return e-mail or by > telephone > on +61-3-9945 7510. In this case, you > should not read, print, re-transmit, > store or act in reliance on this e-mail or > any attachments, and should > destroy all copies of them. This e-mail and > any attachments are > confidential and may contain legally privileged information > and/or > copyright material. You should only re-transmit, distribute or > commercialise the material if you > are authorised to do so. Although we use > antimalware, we disclaim any liability for any malware in any > message or > attachment. This > notice should not be removed. > > ** > > > *IMPORTANT NOTE. *If you are NOT AN AUTHORISED RECIPIENT of this e-mail, > please contact Planet Innovation by return e-mail or by telephone on > +61-3-9945 7510. In this case, you should not read, print, re-transmit, > store or act in reliance on this e-mail or any attachments, and should > destroy all copies of them. This e-mail and any attachments are > confidential and may contain legally privileged information and/or > copyright material. You should only re-transmit, distribute or > commercialise the material if you are authorised to do so. Although we > use antimalware, we disclaim any liability for any malware in any > message or attachment. This notice should not be removed. > > ** > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To post to this group, send email to swupdate@googlegroups.com > <mailto:swupdate@googlegroups.com>. > For more options, visit https://groups.google.com/d/optout.
Hi Austin, On 23/04/19 10:56, Stefano Babic wrote: > Hi Austin, > > On 23/04/19 02:08, 'Austin Phillips' via swupdate wrote: >> Bump. Are there additional changes required to this patch before it can >> be merged? >> > > I left out to check it again, but I do not see any reasons to block it. > I pick it up and I merge for incoming release. ok, now I remember what I found - see later: >> On Tuesday, 2 April 2019 11:12:46 UTC+11, Austin Phillips 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 >> <mailto:austin.phillips@planetinnovation.com.au>> >> --- >> handlers/archive_handler.c | 134 >> ++++++++++++++++++++++++++++++++------------- >> 1 file changed, 95 insertions(+), 39 deletions(-) >> >> diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c >> index a44819d..49c5cad 100644 >> --- a/handlers/archive_handler.c >> +++ b/handlers/archive_handler.c >> @@ -68,15 +68,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; This is used for the return value of pthread_exit(), but the types are different and casting generates warnings: handlers/archive_handler.c:157:15: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] pthread_exit((void *)exitval); Same for status, defined as void *: handlers/archive_handler.c:280:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] else if ((int)status != 0) { Please fix these issues, thanks. Best regards, Stefano Babic >> >> 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 +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,32 +131,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); >> + } >> + >> + 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* 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 +194,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 +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,64 @@ 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); >> - >> - ret = chdir(pwd); >> + if (!thread_ret) { >> + 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]) { >> + 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)) >> -- >> 2.7.4 >> >> >> -- >> >> >> >> >> *IMPORTANT >> NOTE. *If you are >> NOT AN AUTHORISED RECIPIENT of this >> e-mail, please contact Planet Innovation by return e-mail or by >> telephone >> on +61-3-9945 7510. In this case, you >> should not read, print, re-transmit, >> store or act in reliance on this e-mail or >> any attachments, and should >> destroy all copies of them. This e-mail and >> any attachments are >> confidential and may contain legally privileged information >> and/or >> copyright material. You should only re-transmit, distribute or >> commercialise the material if you >> are authorised to do so. Although we use >> antimalware, we disclaim any liability for any malware in any >> message or >> attachment. This >> notice should not be removed. >> >> ** >> >> >> *IMPORTANT NOTE. *If you are NOT AN AUTHORISED RECIPIENT of this e-mail, >> please contact Planet Innovation by return e-mail or by telephone on >> +61-3-9945 7510. In this case, you should not read, print, re-transmit, >> store or act in reliance on this e-mail or any attachments, and should >> destroy all copies of them. This e-mail and any attachments are >> confidential and may contain legally privileged information and/or >> copyright material. You should only re-transmit, distribute or >> commercialise the material if you are authorised to do so. Although we >> use antimalware, we disclaim any liability for any malware in any >> message or attachment. This notice should not be removed. >> >> ** >> >> -- >> You received this message because you are subscribed to the Google >> Groups "swupdate" group. >> To unsubscribe from this group and stop receiving emails from it, send >> an email to swupdate+unsubscribe@googlegroups.com >> <mailto:swupdate+unsubscribe@googlegroups.com>. >> To post to this group, send email to swupdate@googlegroups.com >> <mailto:swupdate@googlegroups.com>. >> For more options, visit https://groups.google.com/d/optout. > >
Hi Stefano, > Hi Austin, > > On 23/04/19 10:56, Stefano Babic wrote: > > Hi Austin, > > > > On 23/04/19 02:08, 'Austin Phillips' via swupdate wrote: > >> Bump. Are there additional changes required to this patch before it > >> can be merged? > >> > > > > I left out to check it again, but I do not see any reasons to block it. > > I pick it up and I merge for incoming release. > > ok, now I remember what I found - see later: > > >> On Tuesday, 2 April 2019 11:12:46 UTC+11, Austin Phillips 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 > >> <mailto:austin.phillips@planetinnovation.com.au>> > >> --- > >> handlers/archive_handler.c | 134 > >> ++++++++++++++++++++++++++++++++------------- > >> 1 file changed, 95 insertions(+), 39 deletions(-) > >> > >> diff --git a/handlers/archive_handler.c > >> b/handlers/archive_handler.c > >> index a44819d..49c5cad 100644 > >> --- a/handlers/archive_handler.c > >> +++ b/handlers/archive_handler.c > >> @@ -68,15 +68,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; > > This is used for the return value of pthread_exit(), but the types are > different and casting generates warnings: > > handlers/archive_handler.c:157:15: warning: cast to pointer from integer > of > different size [-Wint-to-pointer-cast] > pthread_exit((void *)exitval); > > Same for status, defined as void *: > > handlers/archive_handler.c:280:12: warning: cast from pointer to integer > of > different size [-Wpointer-to-int-cast] > else if ((int)status != 0) { > > Please fix these issues, thanks. Fixed and patch v3 sent. Casting the value passed to pthread_exit was a bit of a hack so I've modified the thread input struct to also contain a return value which can be checked by the caller. This keeps responsibility for thread storage lifetime with the caller. If casting were to be retained, the intptr_t type should be used instead of int however this requires C99 and it wasn't clear to me what version of C is being targeted. The call to pthread_exit is probably not needed as the thread will terminate when the thread function returns. A future change could remove this call. > > Best regards, > Stefano Babic > > > >> > >> 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 +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,32 +131,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); > >> + } > >> + > >> + 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* 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 +194,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 +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,64 @@ 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); > >> - > >> - ret = chdir(pwd); > >> + if (!thread_ret) { > >> + 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]) { > >> + 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)) > >> -- > >> 2.7.4 > >> > >> > >> -- > >> > >> > >> > >> > >> *IMPORTANT > >> NOTE. *If you are > >> NOT AN AUTHORISED RECIPIENT of this > >> e-mail, please contact Planet Innovation by return e-mail or by > >> telephone > >> on +61-3-9945 7510. In this case, you > >> should not read, print, re-transmit, > >> store or act in reliance on this e-mail or > >> any attachments, and should > >> destroy all copies of them. This e-mail and > >> any attachments are > >> confidential and may contain legally privileged information > >> and/or > >> copyright material. You should only re-transmit, distribute or > >> commercialise the material if you > >> are authorised to do so. Although we use > >> antimalware, we disclaim any liability for any malware in any > >> message or > >> attachment. This > >> notice should not be removed. > >> > >> ** > >> > >> > >> *IMPORTANT NOTE. *If you are NOT AN AUTHORISED RECIPIENT of this > >> e-mail, please contact Planet Innovation by return e-mail or by > >> telephone on > >> +61-3-9945 7510. In this case, you should not read, print, > >> +re-transmit, > >> store or act in reliance on this e-mail or any attachments, and > >> should destroy all copies of them. This e-mail and any attachments > >> are confidential and may contain legally privileged information > >> and/or copyright material. You should only re-transmit, distribute or > >> commercialise the material if you are authorised to do so. Although > >> we use antimalware, we disclaim any liability for any malware in any > >> message or attachment. This notice should not be removed. > >> > >> ** > >> > >> -- > >> You received this message because you are subscribed to the Google > >> Groups "swupdate" group. > >> To unsubscribe from this group and stop receiving emails from it, > >> send an email to swupdate+unsubscribe@googlegroups.com > >> <mailto:swupdate+unsubscribe@googlegroups.com>. > >> To post to this group, send email to swupdate@googlegroups.com > >> <mailto:swupdate@googlegroups.com>. > >> For more options, visit https://groups.google.com/d/optout. > > > > > > > -- > ========================================================== > =========== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ========================================================== > ===========
diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..49c5cad 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -68,15 +68,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 +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,32 +131,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); + } + + 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* 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 +194,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 +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,64 @@ 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); - - ret = chdir(pwd); + if (!thread_ret) { + 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]) { + 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 | 134 ++++++++++++++++++++++++++++++++------------- 1 file changed, 95 insertions(+), 39 deletions(-)