Message ID | 20210825204957.40150-1-james.hilliard1@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2,1/1] Create and destroy scripts/datadst temp folders on install. | expand |
Hi James, On 25.08.21 22:49, James Hilliard wrote: > Under some circumstances these folders may get removed after > startup, for example if /tmp has a mountpoint change after > swupdate is started. > > In order to make swupdate better able to handle these sort of > race conditions we can create/destroy these folders for each > update installation. > > Fixes: > [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > [ERROR] : SWUPDATE failed [1] Installation failed ! > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Changes v1 -> v2: > - just create/destroy datadst once for each install > --- > core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > core/swupdate.c | 57 -------------------------------------- > 2 files changed, 62 insertions(+), 66 deletions(-) > > diff --git a/core/installer.c b/core/installer.c > index 5e4cbe5..fbb5e5a 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -18,6 +18,7 @@ > #include <sys/types.h> > #include <sys/wait.h> > #include <fcntl.h> > +#include <ftw.h> > #include <sys/stat.h> > #include <sys/mount.h> > > @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > return 0; > } > > +static void create_directory(const char* path) { > + char* dpath; > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > + ENOMEM_ASPRINTF) { > + ERROR("OOM: Directory %s not created", path); > + return; > + } > + if (mkdir(dpath, 0777)) { > + WARN("Directory %s cannot be created due to : %s", > + path, strerror(errno)); > + } > + free(dpath); > +} > + Well, I know they re just used once (up now), but if we move them, why not in util.c and made them public (maybe renaming them, like swupdate_create_directory to avoid possible future conflicts). > +#ifndef CONFIG_NOCLEANUP > +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > + int typeflag, struct FTW *ftwbuf) > +{ > + (void)sb; > + (void)typeflag; > + (void)ftwbuf; > + return remove(fpath); > +} > + > +static int remove_directory(const char* path) > +{ > + char* dpath; > + int ret; > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > + ENOMEM_ASPRINTF) { > + ERROR("OOM: Directory %s not removed", path); > + return -ENOMEM; > + } > + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > + free(dpath); > + return ret; > +} > +#endif > + > int install_single_image(struct img_type *img, bool dry_run) > { > struct installer_handler *hnd; > @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > bool dry_run = sw->parms.dry_run; > bool dropimg; > > + /* Create directories for scripts/datadst */ > + create_directory(SCRIPTS_DIR_SUFFIX); > + create_directory(DATADST_DIR_SUFFIX); > + I still think this is not the correct place. In this way, directories are not created after each transaction is generated, but by its run of a single artifact. IMHO this should be done when SWUpdate is waked up, that is in network_initializer(), that is the main thread. 524 pthread_mutex_unlock(&stream_mutex); 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software Update started !"); 526 TRACE("Software update started"); Current logic was that artifacts must be erased (also due to security reasons because they can contain files after decryption if streaming is not set), but directory, well, they could live. But it is also ok to drop them after a run (nevertheless, I do not consider it as a leak). > /* Extract all scripts, preinstall scripts must be run now */ > const char* tmpdir_scripts = get_tmpdirscripts(); > ret = extract_scripts(&sw->scripts); > ret |= extract_scripts(&sw->bootscripts); > if (ret) { > ERROR("extracting script to %s failed", tmpdir_scripts); > - return ret; > + goto out; > } > > /* Scripts must be run before installing images */ > @@ -272,7 +316,7 @@ int install_images(struct swupdate_cfg *sw) > ret = run_prepost_scripts(&sw->scripts, PREINSTALL); > if (ret) { > ERROR("execute preinstall scripts failed"); > - return ret; > + goto out; > } > } > > @@ -293,14 +337,16 @@ int install_images(struct swupdate_cfg *sw) > if (asprintf(&filename, "%s%s", TMPDIR, img->fname) == > ENOMEM_ASPRINTF) { > ERROR("Path too long: %s%s", TMPDIR, img->fname); > - return -1; > + ret = -1; > + goto out; > } > > ret = stat(filename, &buf); > if (ret) { > TRACE("%s not found or wrong", filename); > free(filename); > - return -1; > + ret = -1; > + goto out; > } > img->size = buf.st_size; > img->fdin = open(filename, O_RDONLY); > @@ -308,7 +354,8 @@ int install_images(struct swupdate_cfg *sw) > if (img->fdin < 0) { > ERROR("Image %s cannot be opened", > img->fname); > - return -1; > + ret = -1; > + goto out; > } > > if ((strlen(img->path) > 0) && > @@ -337,20 +384,20 @@ int install_images(struct swupdate_cfg *sw) > free_image(img); > > if (ret) > - return ret; > + goto out; > } > > /* > * Skip scripts in dry-run mode > */ > if (dry_run) { > - return ret; > + goto out; > } > > ret = run_prepost_scripts(&sw->scripts, POSTINSTALL); > if (ret) { > ERROR("execute postinstall scripts failed"); > - return ret; > + goto out; > } > > if (!LIST_EMPTY(&sw->bootloader)) { > @@ -358,12 +405,18 @@ int install_images(struct swupdate_cfg *sw) > sprintf(bootscript, "%s%s", TMPDIR, BOOT_SCRIPT_SUFFIX); > ret = update_bootloader_env(sw, bootscript); > if (ret) { > - return ret; > + goto out; > } > } > > ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL); > > +out: > +#ifndef CONFIG_NOCLEANUP > + remove_directory(SCRIPTS_DIR_SUFFIX); > + remove_directory(DATADST_DIR_SUFFIX); > +#endif > + > return ret; > } > > diff --git a/core/swupdate.c b/core/swupdate.c > index 949a647..a31aeb1 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -23,7 +23,6 @@ > #include <pthread.h> > #include <signal.h> > #include <sys/wait.h> > -#include <ftw.h> > > #include "bsdqueue.h" > #include "cpiohdr.h" > @@ -244,53 +243,6 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) > return 0; > } > > -static void create_directory(const char* path) { > - char* dpath; > - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > - ENOMEM_ASPRINTF) { > - ERROR("OOM: Directory %s not created", path); > - return; > - } > - if (mkdir(dpath, 0777)) { > - WARN("Directory %s cannot be created due to : %s", > - path, strerror(errno)); > - } > - free(dpath); > -} > - > -#ifndef CONFIG_NOCLEANUP > -static int _remove_directory_cb(const char *fpath, const struct stat *sb, > - int typeflag, struct FTW *ftwbuf) > -{ > - (void)sb; > - (void)typeflag; > - (void)ftwbuf; > - return remove(fpath); > -} > - > -static int remove_directory(const char* path) > -{ > - char* dpath; > - int ret; > - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > - ENOMEM_ASPRINTF) { > - ERROR("OOM: Directory %s not removed", path); > - return -ENOMEM; > - } > - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > - free(dpath); > - return ret; > -} > -#endif > - > -static void swupdate_cleanup(void) > -{ > -#ifndef CONFIG_NOCLEANUP > - remove_directory(SCRIPTS_DIR_SUFFIX); > - remove_directory(DATADST_DIR_SUFFIX); > -#endif > -} > - > static void swupdate_init(struct swupdate_cfg *sw) > { > /* Initialize internal tree to store configuration */ > @@ -303,15 +255,6 @@ static void swupdate_init(struct swupdate_cfg *sw) > LIST_INIT(&sw->extprocs); > sw->cert_purpose = SSL_PURPOSE_DEFAULT; > > - > - /* Create directories for scripts */ > - create_directory(SCRIPTS_DIR_SUFFIX); > - create_directory(DATADST_DIR_SUFFIX); > - > - if (atexit(swupdate_cleanup) != 0) { > - TRACE("Cannot setup SWUpdate cleanup on exit"); > - } > - > #ifdef CONFIG_MTD > mtd_init(); > ubi_init(); > Best regards, Stefano
Hi, > > Under some circumstances these folders may get removed after > > startup, for example if /tmp has a mountpoint change after > > swupdate is started. > > > > In order to make swupdate better able to handle these sort of > > race conditions we can create/destroy these folders for each > > update installation. > > > > Fixes: > > [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > > [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > > [ERROR] : SWUPDATE failed [1] Installation failed ! > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > Changes v1 -> v2: > > - just create/destroy datadst once for each install > > --- > > core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > > core/swupdate.c | 57 -------------------------------------- > > 2 files changed, 62 insertions(+), 66 deletions(-) > > > > diff --git a/core/installer.c b/core/installer.c > > index 5e4cbe5..fbb5e5a 100644 > > --- a/core/installer.c > > +++ b/core/installer.c > > @@ -18,6 +18,7 @@ > > #include <sys/types.h> > > #include <sys/wait.h> > > #include <fcntl.h> > > +#include <ftw.h> > > #include <sys/stat.h> > > #include <sys/mount.h> > > @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > > return 0; > > } > > +static void create_directory(const char* path) { > > + char* dpath; > > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > > + ENOMEM_ASPRINTF) { > > + ERROR("OOM: Directory %s not created", path); > > + return; > > + } > > + if (mkdir(dpath, 0777)) { > > + WARN("Directory %s cannot be created due to : %s", > > + path, strerror(errno)); > > + } > > + free(dpath); > > +} > > + > > Well, I know they re just used once (up now), but if we move them, why not in > util.c and made them public (maybe renaming them, like > swupdate_create_directory to avoid possible future conflicts). > > > +#ifndef CONFIG_NOCLEANUP > > +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > > + int typeflag, struct FTW *ftwbuf) > > +{ > > + (void)sb; > > + (void)typeflag; > > + (void)ftwbuf; > > + return remove(fpath); > > +} > > + > > +static int remove_directory(const char* path) > > +{ > > + char* dpath; > > + int ret; > > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > > + ENOMEM_ASPRINTF) { > > + ERROR("OOM: Directory %s not removed", path); > > + return -ENOMEM; > > + } > > + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > > + free(dpath); > > + return ret; > > +} > > +#endif > > + > > int install_single_image(struct img_type *img, bool dry_run) > > { > > struct installer_handler *hnd; > > @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > > bool dry_run = sw->parms.dry_run; > > bool dropimg; > > + /* Create directories for scripts/datadst */ > > + create_directory(SCRIPTS_DIR_SUFFIX); > > + create_directory(DATADST_DIR_SUFFIX); > > + > > I still think this is not the correct place. In this way, directories are not > created after each transaction is generated, but by its run of a single > artifact. > > IMHO this should be done when SWUpdate is waked up, that is in > network_initializer(), that is the main thread. > > 524 pthread_mutex_unlock(&stream_mutex); > 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > Update started !"); > 526 TRACE("Software update started"); > > Current logic was that artifacts must be erased (also due to security reasons > because they can contain files after decryption if streaming is not set), but > directory, well, they could live. But it is also ok to drop them after a run > (nevertheless, I do not consider it as a leak). I second that, after a transaction, all temporary/extracted files should be removed, for security reasons, and this is what we do have core/installer.c::cleanup_files() for which is called in network_initializer(), way down. What's the added benefit of doing this per artifact over doing this once per transaction, i.e., creating all possibly needed directories prior to a transaction (where Stefano pointed to, as a counterpart of cleanup_files()) which then get removed by core/installer.c::cleanup_files()? Aside, out of curiosity, you assume /tmp mountpoint changes do not happen while SWUpdate is in a transaction, right? Or do you have a measure in place to prevent such changes while a transaction? Kind regards, Christian
On Thu, Aug 26, 2021 at 7:30 AM Christian Storm <christian.storm@siemens.com> wrote: > > Hi, > > > > Under some circumstances these folders may get removed after > > > startup, for example if /tmp has a mountpoint change after > > > swupdate is started. > > > > > > In order to make swupdate better able to handle these sort of > > > race conditions we can create/destroy these folders for each > > > update installation. > > > > > > Fixes: > > > [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > > > [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > > > [ERROR] : SWUPDATE failed [1] Installation failed ! > > > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > --- > > > Changes v1 -> v2: > > > - just create/destroy datadst once for each install > > > --- > > > core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > > > core/swupdate.c | 57 -------------------------------------- > > > 2 files changed, 62 insertions(+), 66 deletions(-) > > > > > > diff --git a/core/installer.c b/core/installer.c > > > index 5e4cbe5..fbb5e5a 100644 > > > --- a/core/installer.c > > > +++ b/core/installer.c > > > @@ -18,6 +18,7 @@ > > > #include <sys/types.h> > > > #include <sys/wait.h> > > > #include <fcntl.h> > > > +#include <ftw.h> > > > #include <sys/stat.h> > > > #include <sys/mount.h> > > > @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > > > return 0; > > > } > > > +static void create_directory(const char* path) { > > > + char* dpath; > > > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > > > + ENOMEM_ASPRINTF) { > > > + ERROR("OOM: Directory %s not created", path); > > > + return; > > > + } > > > + if (mkdir(dpath, 0777)) { > > > + WARN("Directory %s cannot be created due to : %s", > > > + path, strerror(errno)); > > > + } > > > + free(dpath); > > > +} > > > + > > > > Well, I know they re just used once (up now), but if we move them, why not in > > util.c and made them public (maybe renaming them, like > > swupdate_create_directory to avoid possible future conflicts). Ok, moved those back to util.c and renamed them as suggested for v3. > > > > > +#ifndef CONFIG_NOCLEANUP > > > +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > > > + int typeflag, struct FTW *ftwbuf) > > > +{ > > > + (void)sb; > > > + (void)typeflag; > > > + (void)ftwbuf; > > > + return remove(fpath); > > > +} > > > + > > > +static int remove_directory(const char* path) > > > +{ > > > + char* dpath; > > > + int ret; > > > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > > > + ENOMEM_ASPRINTF) { > > > + ERROR("OOM: Directory %s not removed", path); > > > + return -ENOMEM; > > > + } > > > + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > > > + free(dpath); > > > + return ret; > > > +} > > > +#endif > > > + > > > int install_single_image(struct img_type *img, bool dry_run) > > > { > > > struct installer_handler *hnd; > > > @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > > > bool dry_run = sw->parms.dry_run; > > > bool dropimg; > > > + /* Create directories for scripts/datadst */ > > > + create_directory(SCRIPTS_DIR_SUFFIX); > > > + create_directory(DATADST_DIR_SUFFIX); > > > + > > > > I still think this is not the correct place. In this way, directories are not > > created after each transaction is generated, but by its run of a single > > artifact. > > > > IMHO this should be done when SWUpdate is waked up, that is in > > network_initializer(), that is the main thread. Ok, changed in v3. > > > > 524 pthread_mutex_unlock(&stream_mutex); > > 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > > Update started !"); > > 526 TRACE("Software update started"); > > > > Current logic was that artifacts must be erased (also due to security reasons > > because they can contain files after decryption if streaming is not set), but > > directory, well, they could live. But it is also ok to drop them after a run > > (nevertheless, I do not consider it as a leak). Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders after swupdate starts(which is done very early and unconditionally as a service in my setup) but before the update is sent to it. > > I second that, after a transaction, all temporary/extracted files > should be removed, for security reasons, and this is what we do > have core/installer.c::cleanup_files() for which is called in > network_initializer(), way down. > > What's the added benefit of doing this per artifact over doing this once > per transaction, i.e., creating all possibly needed directories prior to > a transaction (where Stefano pointed to, as a counterpart of > cleanup_files()) which then get removed by core/installer.c::cleanup_files()? I had split it into individual handlers initially in v1 to isolate the logic more, but either way should be fine I think, there might be slightly less chance of hitting certain edge cases I was thinking if it's isolated to the handlers, however that could also result in more bugs if a create/remove directory call gets missed as there are more call sites. > > Aside, out of curiosity, you assume /tmp mountpoint changes do not > happen while SWUpdate is in a transaction, right? Or do you have > a measure in place to prevent such changes while a transaction? For the issue I hit at least the most problematic part is that resending the swu on failure wouldn't fix the issue with missing tmp directories as swupdate would only create them on startup, this way at least if the issue gets again somehow a resend for the swu should have a decent chance of automatically fixing the issue by recreating the directories. > > > > Kind regards, > Christian > > -- > Dr. Christian Storm > Siemens AG, Technology, T RDA IOT SES-DE > Otto-Hahn-Ring 6, 81739 München, Germany > > -- > 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. > To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210826133052.txu6egjengdfje4f%40MD1ZFJVC.ad001.siemens.net.
Hi, > [...] > > > > > > 524 pthread_mutex_unlock(&stream_mutex); > > > 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > > > Update started !"); > > > 526 TRACE("Software update started"); > > > > > > Current logic was that artifacts must be erased (also due to security reasons > > > because they can contain files after decryption if streaming is not set), but > > > directory, well, they could live. But it is also ok to drop them after a run > > > (nevertheless, I do not consider it as a leak). > > Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders > after swupdate starts(which is done very early and unconditionally as a service > in my setup) but before the update is sent to it. OK, but this sounds more like an "integrator/environment problem" than a thing SWUpdate can solve ― while it certainly can support you as in functionality in this patch series. One thing, e.g., is to move SWUpdate's (temp/socket) folders to some more stable location, to depend SWUpdate's startup on these preparations being done, or hooking up a post-start to those /tmp changes that recreates SWUpdate's needed folders... Some guarantees by the underlying system have to be taken as granted by SWUpdate, and in this case it's a stable file system backing *while* a transaction ― after having applied this patch's functionality idea. > > I second that, after a transaction, all temporary/extracted files > > should be removed, for security reasons, and this is what we do > > have core/installer.c::cleanup_files() for which is called in > > network_initializer(), way down. > > > > What's the added benefit of doing this per artifact over doing this once > > per transaction, i.e., creating all possibly needed directories prior to > > a transaction (where Stefano pointed to, as a counterpart of > > cleanup_files()) which then get removed by core/installer.c::cleanup_files()? > > I had split it into individual handlers initially in v1 to isolate the > logic more, but > either way should be fine I think, there might be slightly less chance > of hitting > certain edge cases I was thinking if it's isolated to the handlers, however that > could also result in more bugs if a create/remove directory call gets missed as > there are more call sites. Yes, I second that. > > Aside, out of curiosity, you assume /tmp mountpoint changes do not > > happen while SWUpdate is in a transaction, right? Or do you have > > a measure in place to prevent such changes while a transaction? > > For the issue I hit at least the most problematic part is that resending the swu > on failure wouldn't fix the issue with missing tmp directories as swupdate > would only create them on startup, this way at least if the issue gets again > somehow a resend for the swu should have a decent chance of automatically > fixing the issue by recreating the directories. I see. If you now assume that such changes can occur anytime at runtime, you're at the problem to give SWUpdate a stable file system backing not only in between but also while a transaction. The former case is done with this patch series functionality while the later case is harder to accomplish ― if that's considered to be SWUpdate's job/problem at all. Kind regards, Christian
Hi James, On 27.08.21 07:03, James Hilliard wrote: > On Thu, Aug 26, 2021 at 7:30 AM Christian Storm > <christian.storm@siemens.com> wrote: >> >> Hi, >> >>>> Under some circumstances these folders may get removed after >>>> startup, for example if /tmp has a mountpoint change after >>>> swupdate is started. >>>> >>>> In order to make swupdate better able to handle these sort of >>>> race conditions we can create/destroy these folders for each >>>> update installation. >>>> >>>> Fixes: >>>> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 >>>> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed >>>> [ERROR] : SWUPDATE failed [1] Installation failed ! >>>> >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>> --- >>>> Changes v1 -> v2: >>>> - just create/destroy datadst once for each install >>>> --- >>>> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ >>>> core/swupdate.c | 57 -------------------------------------- >>>> 2 files changed, 62 insertions(+), 66 deletions(-) >>>> >>>> diff --git a/core/installer.c b/core/installer.c >>>> index 5e4cbe5..fbb5e5a 100644 >>>> --- a/core/installer.c >>>> +++ b/core/installer.c >>>> @@ -18,6 +18,7 @@ >>>> #include <sys/types.h> >>>> #include <sys/wait.h> >>>> #include <fcntl.h> >>>> +#include <ftw.h> >>>> #include <sys/stat.h> >>>> #include <sys/mount.h> >>>> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) >>>> return 0; >>>> } >>>> +static void create_directory(const char* path) { >>>> + char* dpath; >>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >>>> + ENOMEM_ASPRINTF) { >>>> + ERROR("OOM: Directory %s not created", path); >>>> + return; >>>> + } >>>> + if (mkdir(dpath, 0777)) { >>>> + WARN("Directory %s cannot be created due to : %s", >>>> + path, strerror(errno)); >>>> + } >>>> + free(dpath); >>>> +} >>>> + >>> >>> Well, I know they re just used once (up now), but if we move them, why not in >>> util.c and made them public (maybe renaming them, like >>> swupdate_create_directory to avoid possible future conflicts). > > Ok, moved those back to util.c and renamed them as suggested for v3. > >>> >>>> +#ifndef CONFIG_NOCLEANUP >>>> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, >>>> + int typeflag, struct FTW *ftwbuf) >>>> +{ >>>> + (void)sb; >>>> + (void)typeflag; >>>> + (void)ftwbuf; >>>> + return remove(fpath); >>>> +} >>>> + >>>> +static int remove_directory(const char* path) >>>> +{ >>>> + char* dpath; >>>> + int ret; >>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >>>> + ENOMEM_ASPRINTF) { >>>> + ERROR("OOM: Directory %s not removed", path); >>>> + return -ENOMEM; >>>> + } >>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >>>> + free(dpath); >>>> + return ret; >>>> +} >>>> +#endif >>>> + >>>> int install_single_image(struct img_type *img, bool dry_run) >>>> { >>>> struct installer_handler *hnd; >>>> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) >>>> bool dry_run = sw->parms.dry_run; >>>> bool dropimg; >>>> + /* Create directories for scripts/datadst */ >>>> + create_directory(SCRIPTS_DIR_SUFFIX); >>>> + create_directory(DATADST_DIR_SUFFIX); >>>> + >>> >>> I still think this is not the correct place. In this way, directories are not >>> created after each transaction is generated, but by its run of a single >>> artifact. >>> >>> IMHO this should be done when SWUpdate is waked up, that is in >>> network_initializer(), that is the main thread. > > Ok, changed in v3. > >>> >>> 524 pthread_mutex_unlock(&stream_mutex); >>> 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software >>> Update started !"); >>> 526 TRACE("Software update started"); >>> >>> Current logic was that artifacts must be erased (also due to security reasons >>> because they can contain files after decryption if streaming is not set), but >>> directory, well, they could live. But it is also ok to drop them after a run >>> (nevertheless, I do not consider it as a leak). > > Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders > after swupdate starts(which is done very early and unconditionally as a service > in my setup) but before the update is sent to it. The topöic with missing dirs was also raised several times - so it is fine with me to inmtegrate your patch. However, it was discovered in the past (and due to the fact that directories were created just once), that the issue was created by systemd. Is it not maybe your case, too ? See config files for systemd in meta-swupdate: https://github.com/sbabic/meta-swupdate/blob/master/recipes-support/swupdate/swupdate/tmpfiles-swupdate.conf > >> >> I second that, after a transaction, all temporary/extracted files >> should be removed, for security reasons, and this is what we do >> have core/installer.c::cleanup_files() for which is called in >> network_initializer(), way down. >> >> What's the added benefit of doing this per artifact over doing this once >> per transaction, i.e., creating all possibly needed directories prior to >> a transaction (where Stefano pointed to, as a counterpart of >> cleanup_files()) which then get removed by core/installer.c::cleanup_files()? > > I had split it into individual handlers initially in v1 to isolate the > logic more, but > either way should be fine I think, there might be slightly less chance > of hitting > certain edge cases I was thinking if it's isolated to the handlers, however that > could also result in more bugs if a create/remove directory call gets missed as > there are more call sites. > >> >> Aside, out of curiosity, you assume /tmp mountpoint changes do not >> happen while SWUpdate is in a transaction, right? Or do you have >> a measure in place to prevent such changes while a transaction? > > For the issue I hit at least the most problematic part is that resending the swu > on failure wouldn't fix the issue with missing tmp directories as swupdate > would only create them on startup, this way at least if the issue gets again > somehow a resend for the swu should have a decent chance of automatically > fixing the issue by recreating the directories. Ok, let's get this in. My only fear (as in the past) is that this hides the real cause (it was systemd). It is clear to me that deletion of directories is not done by SWUpdate, but from an external agent. Best regards, Stefano > >> >> >> >> Kind regards, >> Christian >> >> -- >> Dr. Christian Storm >> Siemens AG, Technology, T RDA IOT SES-DE >> Otto-Hahn-Ring 6, 81739 München, Germany >> >> -- >> 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. >> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210826133052.txu6egjengdfje4f%40MD1ZFJVC.ad001.siemens.net. >
On Fri, Aug 27, 2021 at 1:44 AM Christian Storm <christian.storm@siemens.com> wrote: > > Hi, > > > [...] > > > > > > > > 524 pthread_mutex_unlock(&stream_mutex); > > > > 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > > > > Update started !"); > > > > 526 TRACE("Software update started"); > > > > > > > > Current logic was that artifacts must be erased (also due to security reasons > > > > because they can contain files after decryption if streaming is not set), but > > > > directory, well, they could live. But it is also ok to drop them after a run > > > > (nevertheless, I do not consider it as a leak). > > > > Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders > > after swupdate starts(which is done very early and unconditionally as a service > > in my setup) but before the update is sent to it. > > OK, but this sounds more like an "integrator/environment problem" than > a thing SWUpdate can solve ― while it certainly can support you as in > functionality in this patch series. One thing, e.g., is to move > SWUpdate's (temp/socket) folders to some more stable location, to > depend SWUpdate's startup on these preparations being done, or hooking > up a post-start to those /tmp changes that recreates SWUpdate's needed > folders... > > Some guarantees by the underlying system have to be taken as granted by > SWUpdate, and in this case it's a stable file system backing *while* > a transaction ― after having applied this patch's functionality idea. Yeah, this should be sufficient for resolving the issue I'm hitting at least. > > > > > I second that, after a transaction, all temporary/extracted files > > > should be removed, for security reasons, and this is what we do > > > have core/installer.c::cleanup_files() for which is called in > > > network_initializer(), way down. > > > > > > What's the added benefit of doing this per artifact over doing this once > > > per transaction, i.e., creating all possibly needed directories prior to > > > a transaction (where Stefano pointed to, as a counterpart of > > > cleanup_files()) which then get removed by core/installer.c::cleanup_files()? > > > > I had split it into individual handlers initially in v1 to isolate the > > logic more, but > > either way should be fine I think, there might be slightly less chance > > of hitting > > certain edge cases I was thinking if it's isolated to the handlers, however that > > could also result in more bugs if a create/remove directory call gets missed as > > there are more call sites. > > Yes, I second that. > > > > Aside, out of curiosity, you assume /tmp mountpoint changes do not > > > happen while SWUpdate is in a transaction, right? Or do you have > > > a measure in place to prevent such changes while a transaction? > > > > For the issue I hit at least the most problematic part is that resending the swu > > on failure wouldn't fix the issue with missing tmp directories as swupdate > > would only create them on startup, this way at least if the issue gets again > > somehow a resend for the swu should have a decent chance of automatically > > fixing the issue by recreating the directories. > > I see. If you now assume that such changes can occur anytime at runtime, > you're at the problem to give SWUpdate a stable file system backing not > only in between but also while a transaction. The former case is done > with this patch series functionality while the later case is harder to > accomplish ― if that's considered to be SWUpdate's job/problem at all. Yeah, I haven't seen any issues occurring after initial boot really, I only hit the other one with swupdate running as a systemd service on bootup. Manually restarting the swupdate service would always fix the error without this change at least from what I recall. > > > Kind regards, > Christian > > -- > Dr. Christian Storm > Siemens AG, Technology, T RDA IOT SES-DE > Otto-Hahn-Ring 6, 81739 München, Germany > > -- > 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. > To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210827074411.ja43gjpyuq5eav7a%40MD1ZFJVC.ad001.siemens.net.
On Fri, Aug 27, 2021 at 3:46 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 27.08.21 07:03, James Hilliard wrote: > > On Thu, Aug 26, 2021 at 7:30 AM Christian Storm > > <christian.storm@siemens.com> wrote: > >> > >> Hi, > >> > >>>> Under some circumstances these folders may get removed after > >>>> startup, for example if /tmp has a mountpoint change after > >>>> swupdate is started. > >>>> > >>>> In order to make swupdate better able to handle these sort of > >>>> race conditions we can create/destroy these folders for each > >>>> update installation. > >>>> > >>>> Fixes: > >>>> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > >>>> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > >>>> [ERROR] : SWUPDATE failed [1] Installation failed ! > >>>> > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>>> --- > >>>> Changes v1 -> v2: > >>>> - just create/destroy datadst once for each install > >>>> --- > >>>> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > >>>> core/swupdate.c | 57 -------------------------------------- > >>>> 2 files changed, 62 insertions(+), 66 deletions(-) > >>>> > >>>> diff --git a/core/installer.c b/core/installer.c > >>>> index 5e4cbe5..fbb5e5a 100644 > >>>> --- a/core/installer.c > >>>> +++ b/core/installer.c > >>>> @@ -18,6 +18,7 @@ > >>>> #include <sys/types.h> > >>>> #include <sys/wait.h> > >>>> #include <fcntl.h> > >>>> +#include <ftw.h> > >>>> #include <sys/stat.h> > >>>> #include <sys/mount.h> > >>>> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > >>>> return 0; > >>>> } > >>>> +static void create_directory(const char* path) { > >>>> + char* dpath; > >>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >>>> + ENOMEM_ASPRINTF) { > >>>> + ERROR("OOM: Directory %s not created", path); > >>>> + return; > >>>> + } > >>>> + if (mkdir(dpath, 0777)) { > >>>> + WARN("Directory %s cannot be created due to : %s", > >>>> + path, strerror(errno)); > >>>> + } > >>>> + free(dpath); > >>>> +} > >>>> + > >>> > >>> Well, I know they re just used once (up now), but if we move them, why not in > >>> util.c and made them public (maybe renaming them, like > >>> swupdate_create_directory to avoid possible future conflicts). > > > > Ok, moved those back to util.c and renamed them as suggested for v3. > > > >>> > >>>> +#ifndef CONFIG_NOCLEANUP > >>>> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > >>>> + int typeflag, struct FTW *ftwbuf) > >>>> +{ > >>>> + (void)sb; > >>>> + (void)typeflag; > >>>> + (void)ftwbuf; > >>>> + return remove(fpath); > >>>> +} > >>>> + > >>>> +static int remove_directory(const char* path) > >>>> +{ > >>>> + char* dpath; > >>>> + int ret; > >>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >>>> + ENOMEM_ASPRINTF) { > >>>> + ERROR("OOM: Directory %s not removed", path); > >>>> + return -ENOMEM; > >>>> + } > >>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > >>>> + free(dpath); > >>>> + return ret; > >>>> +} > >>>> +#endif > >>>> + > >>>> int install_single_image(struct img_type *img, bool dry_run) > >>>> { > >>>> struct installer_handler *hnd; > >>>> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > >>>> bool dry_run = sw->parms.dry_run; > >>>> bool dropimg; > >>>> + /* Create directories for scripts/datadst */ > >>>> + create_directory(SCRIPTS_DIR_SUFFIX); > >>>> + create_directory(DATADST_DIR_SUFFIX); > >>>> + > >>> > >>> I still think this is not the correct place. In this way, directories are not > >>> created after each transaction is generated, but by its run of a single > >>> artifact. > >>> > >>> IMHO this should be done when SWUpdate is waked up, that is in > >>> network_initializer(), that is the main thread. > > > > Ok, changed in v3. > > > >>> > >>> 524 pthread_mutex_unlock(&stream_mutex); > >>> 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > >>> Update started !"); > >>> 526 TRACE("Software update started"); > >>> > >>> Current logic was that artifacts must be erased (also due to security reasons > >>> because they can contain files after decryption if streaming is not set), but > >>> directory, well, they could live. But it is also ok to drop them after a run > >>> (nevertheless, I do not consider it as a leak). > > > > Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders > > after swupdate starts(which is done very early and unconditionally as a service > > in my setup) but before the update is sent to it. > > The topöic with missing dirs was also raised several times - so it is > fine with me to inmtegrate your patch. > > However, it was discovered in the past (and due to the fact that > directories were created just once), that the issue was created by > systemd. Is it not maybe your case, too ? See config files for systemd > in meta-swupdate: > > https://github.com/sbabic/meta-swupdate/blob/master/recipes-support/swupdate/swupdate/tmpfiles-swupdate.conf Yeah, I bet that's it, I don't use openembedded/yocto and wrote my own service file so I don't think I have that in my build. I'll review that and some of the other service files in meta-swupdate and submit them upstream for the buildroot swupdate package. > > > > >> > >> I second that, after a transaction, all temporary/extracted files > >> should be removed, for security reasons, and this is what we do > >> have core/installer.c::cleanup_files() for which is called in > >> network_initializer(), way down. > >> > >> What's the added benefit of doing this per artifact over doing this once > >> per transaction, i.e., creating all possibly needed directories prior to > >> a transaction (where Stefano pointed to, as a counterpart of > >> cleanup_files()) which then get removed by core/installer.c::cleanup_files()? > > > > I had split it into individual handlers initially in v1 to isolate the > > logic more, but > > either way should be fine I think, there might be slightly less chance > > of hitting > > certain edge cases I was thinking if it's isolated to the handlers, however that > > could also result in more bugs if a create/remove directory call gets missed as > > there are more call sites. > > >> > >> Aside, out of curiosity, you assume /tmp mountpoint changes do not > >> happen while SWUpdate is in a transaction, right? Or do you have > >> a measure in place to prevent such changes while a transaction? > > > > For the issue I hit at least the most problematic part is that resending the swu > > on failure wouldn't fix the issue with missing tmp directories as swupdate > > would only create them on startup, this way at least if the issue gets again > > somehow a resend for the swu should have a decent chance of automatically > > fixing the issue by recreating the directories. > > Ok, let's get this in. My only fear (as in the past) is that this hides > the real cause (it was systemd). It is clear to me that deletion of > directories is not done by SWUpdate, but from an external agent. Yeah, I think this is still a good idea either way. > > Best regards, > Stefano > > > > > >> > >> > >> > >> Kind regards, > >> Christian > >> > >> -- > >> Dr. Christian Storm > >> Siemens AG, Technology, T RDA IOT SES-DE > >> Otto-Hahn-Ring 6, 81739 München, Germany > >> > >> -- > >> 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. > >> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210826133052.txu6egjengdfje4f%40MD1ZFJVC.ad001.siemens.net. > > > > > -- > ===================================================================== > 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 > =====================================================================
On 27.08.21 09:44, Christian Storm wrote: > Hi, > >> [...] >>>> >>>> 524 pthread_mutex_unlock(&stream_mutex); >>>> 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software >>>> Update started !"); >>>> 526 TRACE("Software update started"); >>>> >>>> Current logic was that artifacts must be erased (also due to security reasons >>>> because they can contain files after decryption if streaming is not set), but >>>> directory, well, they could live. But it is also ok to drop them after a run >>>> (nevertheless, I do not consider it as a leak). >> >> Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders >> after swupdate starts(which is done very early and unconditionally as a service >> in my setup) but before the update is sent to it. > > OK, but this sounds more like an "integrator/environment problem" than > a thing SWUpdate can solve Right. > ― while it certainly can support you as in > functionality in this patch series. One thing, e.g., is to move > SWUpdate's (temp/socket) folders to some more stable location, But is this not yet done ? Sockets can be definbed statically (via CONFIG_SOCKET_) or dinamically, because they are put into ${TMPDIR}. It is enough to set TMPDIR before starting SWUpdate to point to a stable location, like /var/run/swupdate or whatever. I use this to run multiple instance of SWUpdate on a single device and it works flawlessly. > to > depend SWUpdate's startup on these preparations being done, or hooking > up a post-start to those /tmp changes that recreates SWUpdate's needed > folders... By the way, this is an attempt to make SWUpdate too smart. Each service depends on a working filesystem. It is not worth to mention that if I run a "rm -f ..." whatever, I do not only damage the filesystem, but I affect many services that stop working. If someone is dropping SWUpdate's directories, it should be duty of the integrator to set correct rights to the directories so that deleting them is not possible, or identify the cause and block it. This is just as thought, it does not mean that I won't merge James' patch. But I am convinced this does not solve a problem is SWUpdate, but a bug somewhere else. > > Some guarantees by the underlying system have to be taken as granted by > SWUpdate, and in this case it's a stable file system backing *while* > a transaction ― after having applied this patch's functionality idea. > > Ok >>> I second that, after a transaction, all temporary/extracted files >>> should be removed, for security reasons, and this is what we do >>> have core/installer.c::cleanup_files() for which is called in >>> network_initializer(), way down. >>> >>> What's the added benefit of doing this per artifact over doing this once >>> per transaction, i.e., creating all possibly needed directories prior to >>> a transaction (where Stefano pointed to, as a counterpart of >>> cleanup_files()) which then get removed by core/installer.c::cleanup_files()? >> >> I had split it into individual handlers initially in v1 to isolate the >> logic more, but >> either way should be fine I think, there might be slightly less chance >> of hitting >> certain edge cases I was thinking if it's isolated to the handlers, however that >> could also result in more bugs if a create/remove directory call gets missed as >> there are more call sites. > > Yes, I second that. Ok > >>> Aside, out of curiosity, you assume /tmp mountpoint changes do not >>> happen while SWUpdate is in a transaction, right? Or do you have >>> a measure in place to prevent such changes while a transaction? >> >> For the issue I hit at least the most problematic part is that resending the swu >> on failure wouldn't fix the issue with missing tmp directories as swupdate >> would only create them on startup, this way at least if the issue gets again >> somehow a resend for the swu should have a decent chance of automatically >> fixing the issue by recreating the directories. > > I see. If you now assume that such changes can occur anytime at runtime, > you're at the problem to give SWUpdate a stable file system backing not > only in between but also while a transaction. The former case is done > with this patch series functionality while the later case is harder to > accomplish ― if that's considered to be SWUpdate's job/problem at all. I do not think this is a bug in SWUpdate. Each service will create resources, and it should rely that these resources are under its control - in James' case, someone else takes the control and decides itself (???) to drop directories that does not belong to it. Best regards, Stefano
Hi James, On 27.08.21 11:56, James Hilliard wrote: > On Fri, Aug 27, 2021 at 3:46 AM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi James, >> >> On 27.08.21 07:03, James Hilliard wrote: >>> On Thu, Aug 26, 2021 at 7:30 AM Christian Storm >>> <christian.storm@siemens.com> wrote: >>>> >>>> Hi, >>>> >>>>>> Under some circumstances these folders may get removed after >>>>>> startup, for example if /tmp has a mountpoint change after >>>>>> swupdate is started. >>>>>> >>>>>> In order to make swupdate better able to handle these sort of >>>>>> race conditions we can create/destroy these folders for each >>>>>> update installation. >>>>>> >>>>>> Fixes: >>>>>> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 >>>>>> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed >>>>>> [ERROR] : SWUPDATE failed [1] Installation failed ! >>>>>> >>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>>>> --- >>>>>> Changes v1 -> v2: >>>>>> - just create/destroy datadst once for each install >>>>>> --- >>>>>> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ >>>>>> core/swupdate.c | 57 -------------------------------------- >>>>>> 2 files changed, 62 insertions(+), 66 deletions(-) >>>>>> >>>>>> diff --git a/core/installer.c b/core/installer.c >>>>>> index 5e4cbe5..fbb5e5a 100644 >>>>>> --- a/core/installer.c >>>>>> +++ b/core/installer.c >>>>>> @@ -18,6 +18,7 @@ >>>>>> #include <sys/types.h> >>>>>> #include <sys/wait.h> >>>>>> #include <fcntl.h> >>>>>> +#include <ftw.h> >>>>>> #include <sys/stat.h> >>>>>> #include <sys/mount.h> >>>>>> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) >>>>>> return 0; >>>>>> } >>>>>> +static void create_directory(const char* path) { >>>>>> + char* dpath; >>>>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >>>>>> + ENOMEM_ASPRINTF) { >>>>>> + ERROR("OOM: Directory %s not created", path); >>>>>> + return; >>>>>> + } >>>>>> + if (mkdir(dpath, 0777)) { >>>>>> + WARN("Directory %s cannot be created due to : %s", >>>>>> + path, strerror(errno)); >>>>>> + } >>>>>> + free(dpath); >>>>>> +} >>>>>> + >>>>> >>>>> Well, I know they re just used once (up now), but if we move them, why not in >>>>> util.c and made them public (maybe renaming them, like >>>>> swupdate_create_directory to avoid possible future conflicts). >>> >>> Ok, moved those back to util.c and renamed them as suggested for v3. >>> >>>>> >>>>>> +#ifndef CONFIG_NOCLEANUP >>>>>> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, >>>>>> + int typeflag, struct FTW *ftwbuf) >>>>>> +{ >>>>>> + (void)sb; >>>>>> + (void)typeflag; >>>>>> + (void)ftwbuf; >>>>>> + return remove(fpath); >>>>>> +} >>>>>> + >>>>>> +static int remove_directory(const char* path) >>>>>> +{ >>>>>> + char* dpath; >>>>>> + int ret; >>>>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >>>>>> + ENOMEM_ASPRINTF) { >>>>>> + ERROR("OOM: Directory %s not removed", path); >>>>>> + return -ENOMEM; >>>>>> + } >>>>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >>>>>> + free(dpath); >>>>>> + return ret; >>>>>> +} >>>>>> +#endif >>>>>> + >>>>>> int install_single_image(struct img_type *img, bool dry_run) >>>>>> { >>>>>> struct installer_handler *hnd; >>>>>> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) >>>>>> bool dry_run = sw->parms.dry_run; >>>>>> bool dropimg; >>>>>> + /* Create directories for scripts/datadst */ >>>>>> + create_directory(SCRIPTS_DIR_SUFFIX); >>>>>> + create_directory(DATADST_DIR_SUFFIX); >>>>>> + >>>>> >>>>> I still think this is not the correct place. In this way, directories are not >>>>> created after each transaction is generated, but by its run of a single >>>>> artifact. >>>>> >>>>> IMHO this should be done when SWUpdate is waked up, that is in >>>>> network_initializer(), that is the main thread. >>> >>> Ok, changed in v3. >>> >>>>> >>>>> 524 pthread_mutex_unlock(&stream_mutex); >>>>> 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software >>>>> Update started !"); >>>>> 526 TRACE("Software update started"); >>>>> >>>>> Current logic was that artifacts must be erased (also due to security reasons >>>>> because they can contain files after decryption if streaming is not set), but >>>>> directory, well, they could live. But it is also ok to drop them after a run >>>>> (nevertheless, I do not consider it as a leak). >>> >>> Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders >>> after swupdate starts(which is done very early and unconditionally as a service >>> in my setup) but before the update is sent to it. >> >> The topöic with missing dirs was also raised several times - so it is >> fine with me to inmtegrate your patch. >> >> However, it was discovered in the past (and due to the fact that >> directories were created just once), that the issue was created by >> systemd. Is it not maybe your case, too ? See config files for systemd >> in meta-swupdate: >> >> https://github.com/sbabic/meta-swupdate/blob/master/recipes-support/swupdate/swupdate/tmpfiles-swupdate.conf > > Yeah, I bet that's it, I don't use openembedded/yocto and wrote my own service > file so I don't think I have that in my build. > Then it is - it was proven in the past that systemd decides itself to drop SWUpdate's files, you find the threads in archive. At that time, I rejected a patch like yours to get better investigated where is the cause. I am not very fond of this systemd's "feature", but there is at least a way to block it. Or the integrator sets TMPDIR to another path that is not destroyed by systemd. > I'll review that and some of the other service files in meta-swupdate and submit > them upstream for the buildroot swupdate package. Nice, thanks for this. > >> >>> >>>> >>>> I second that, after a transaction, all temporary/extracted files >>>> should be removed, for security reasons, and this is what we do >>>> have core/installer.c::cleanup_files() for which is called in >>>> network_initializer(), way down. >>>> >>>> What's the added benefit of doing this per artifact over doing this once >>>> per transaction, i.e., creating all possibly needed directories prior to >>>> a transaction (where Stefano pointed to, as a counterpart of >>>> cleanup_files()) which then get removed by core/installer.c::cleanup_files()? >>> >>> I had split it into individual handlers initially in v1 to isolate the >>> logic more, but >>> either way should be fine I think, there might be slightly less chance >>> of hitting >>> certain edge cases I was thinking if it's isolated to the handlers, however that >>> could also result in more bugs if a create/remove directory call gets missed as >>> there are more call sites. > >>>> >>>> Aside, out of curiosity, you assume /tmp mountpoint changes do not >>>> happen while SWUpdate is in a transaction, right? Or do you have >>>> a measure in place to prevent such changes while a transaction? >>> >>> For the issue I hit at least the most problematic part is that resending the swu >>> on failure wouldn't fix the issue with missing tmp directories as swupdate >>> would only create them on startup, this way at least if the issue gets again >>> somehow a resend for the swu should have a decent chance of automatically >>> fixing the issue by recreating the directories. >> >> Ok, let's get this in. My only fear (as in the past) is that this hides >> the real cause (it was systemd). It is clear to me that deletion of >> directories is not done by SWUpdate, but from an external agent. > > Yeah, I think this is still a good idea either way. Best regards, Stefano
On Fri, Aug 27, 2021 at 4:04 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 27.08.21 11:56, James Hilliard wrote: > > On Fri, Aug 27, 2021 at 3:46 AM Stefano Babic <sbabic@denx.de> wrote: > >> > >> Hi James, > >> > >> On 27.08.21 07:03, James Hilliard wrote: > >>> On Thu, Aug 26, 2021 at 7:30 AM Christian Storm > >>> <christian.storm@siemens.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>>>> Under some circumstances these folders may get removed after > >>>>>> startup, for example if /tmp has a mountpoint change after > >>>>>> swupdate is started. > >>>>>> > >>>>>> In order to make swupdate better able to handle these sort of > >>>>>> race conditions we can create/destroy these folders for each > >>>>>> update installation. > >>>>>> > >>>>>> Fixes: > >>>>>> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > >>>>>> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > >>>>>> [ERROR] : SWUPDATE failed [1] Installation failed ! > >>>>>> > >>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>>>>> --- > >>>>>> Changes v1 -> v2: > >>>>>> - just create/destroy datadst once for each install > >>>>>> --- > >>>>>> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > >>>>>> core/swupdate.c | 57 -------------------------------------- > >>>>>> 2 files changed, 62 insertions(+), 66 deletions(-) > >>>>>> > >>>>>> diff --git a/core/installer.c b/core/installer.c > >>>>>> index 5e4cbe5..fbb5e5a 100644 > >>>>>> --- a/core/installer.c > >>>>>> +++ b/core/installer.c > >>>>>> @@ -18,6 +18,7 @@ > >>>>>> #include <sys/types.h> > >>>>>> #include <sys/wait.h> > >>>>>> #include <fcntl.h> > >>>>>> +#include <ftw.h> > >>>>>> #include <sys/stat.h> > >>>>>> #include <sys/mount.h> > >>>>>> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > >>>>>> return 0; > >>>>>> } > >>>>>> +static void create_directory(const char* path) { > >>>>>> + char* dpath; > >>>>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >>>>>> + ENOMEM_ASPRINTF) { > >>>>>> + ERROR("OOM: Directory %s not created", path); > >>>>>> + return; > >>>>>> + } > >>>>>> + if (mkdir(dpath, 0777)) { > >>>>>> + WARN("Directory %s cannot be created due to : %s", > >>>>>> + path, strerror(errno)); > >>>>>> + } > >>>>>> + free(dpath); > >>>>>> +} > >>>>>> + > >>>>> > >>>>> Well, I know they re just used once (up now), but if we move them, why not in > >>>>> util.c and made them public (maybe renaming them, like > >>>>> swupdate_create_directory to avoid possible future conflicts). > >>> > >>> Ok, moved those back to util.c and renamed them as suggested for v3. > >>> > >>>>> > >>>>>> +#ifndef CONFIG_NOCLEANUP > >>>>>> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > >>>>>> + int typeflag, struct FTW *ftwbuf) > >>>>>> +{ > >>>>>> + (void)sb; > >>>>>> + (void)typeflag; > >>>>>> + (void)ftwbuf; > >>>>>> + return remove(fpath); > >>>>>> +} > >>>>>> + > >>>>>> +static int remove_directory(const char* path) > >>>>>> +{ > >>>>>> + char* dpath; > >>>>>> + int ret; > >>>>>> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >>>>>> + ENOMEM_ASPRINTF) { > >>>>>> + ERROR("OOM: Directory %s not removed", path); > >>>>>> + return -ENOMEM; > >>>>>> + } > >>>>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > >>>>>> + free(dpath); > >>>>>> + return ret; > >>>>>> +} > >>>>>> +#endif > >>>>>> + > >>>>>> int install_single_image(struct img_type *img, bool dry_run) > >>>>>> { > >>>>>> struct installer_handler *hnd; > >>>>>> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > >>>>>> bool dry_run = sw->parms.dry_run; > >>>>>> bool dropimg; > >>>>>> + /* Create directories for scripts/datadst */ > >>>>>> + create_directory(SCRIPTS_DIR_SUFFIX); > >>>>>> + create_directory(DATADST_DIR_SUFFIX); > >>>>>> + > >>>>> > >>>>> I still think this is not the correct place. In this way, directories are not > >>>>> created after each transaction is generated, but by its run of a single > >>>>> artifact. > >>>>> > >>>>> IMHO this should be done when SWUpdate is waked up, that is in > >>>>> network_initializer(), that is the main thread. > >>> > >>> Ok, changed in v3. > >>> > >>>>> > >>>>> 524 pthread_mutex_unlock(&stream_mutex); > >>>>> 525 notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software > >>>>> Update started !"); > >>>>> 526 TRACE("Software update started"); > >>>>> > >>>>> Current logic was that artifacts must be erased (also due to security reasons > >>>>> because they can contain files after decryption if streaming is not set), but > >>>>> directory, well, they could live. But it is also ok to drop them after a run > >>>>> (nevertheless, I do not consider it as a leak). > >>> > >>> Yeah, this is mostly to reduce the chance of stuff messing with the tmp folders > >>> after swupdate starts(which is done very early and unconditionally as a service > >>> in my setup) but before the update is sent to it. > >> > >> The topöic with missing dirs was also raised several times - so it is > >> fine with me to inmtegrate your patch. > >> > >> However, it was discovered in the past (and due to the fact that > >> directories were created just once), that the issue was created by > >> systemd. Is it not maybe your case, too ? See config files for systemd > >> in meta-swupdate: > >> > >> https://github.com/sbabic/meta-swupdate/blob/master/recipes-support/swupdate/swupdate/tmpfiles-swupdate.conf > > > > Yeah, I bet that's it, I don't use openembedded/yocto and wrote my own service > > file so I don't think I have that in my build. > > > > Then it is - it was proven in the past that systemd decides itself to > drop SWUpdate's files, you find the threads in archive. At that time, I > rejected a patch like yours to get better investigated where is the cause. I guess the main benefit here is that it makes swupdate more resilient by reducing the amount of dynamic initialization state preserved between transactions. > > I am not very fond of this systemd's "feature", but there is at least a > way to block it. Or the integrator sets TMPDIR to another path that is > not destroyed by systemd. Yeah, I do find it doesn't seem to integrate all that nicely with other parts of systemd. > > > I'll review that and some of the other service files in meta-swupdate and submit > > them upstream for the buildroot swupdate package. > > Nice, thanks for this. > > > > >> > >>> > >>>> > >>>> I second that, after a transaction, all temporary/extracted files > >>>> should be removed, for security reasons, and this is what we do > >>>> have core/installer.c::cleanup_files() for which is called in > >>>> network_initializer(), way down. > >>>> > >>>> What's the added benefit of doing this per artifact over doing this once > >>>> per transaction, i.e., creating all possibly needed directories prior to > >>>> a transaction (where Stefano pointed to, as a counterpart of > >>>> cleanup_files()) which then get removed by core/installer.c::cleanup_files()? > >>> > >>> I had split it into individual handlers initially in v1 to isolate the > >>> logic more, but > >>> either way should be fine I think, there might be slightly less chance > >>> of hitting > >>> certain edge cases I was thinking if it's isolated to the handlers, however that > >>> could also result in more bugs if a create/remove directory call gets missed as > >>> there are more call sites. > > >>>> > >>>> Aside, out of curiosity, you assume /tmp mountpoint changes do not > >>>> happen while SWUpdate is in a transaction, right? Or do you have > >>>> a measure in place to prevent such changes while a transaction? > >>> > >>> For the issue I hit at least the most problematic part is that resending the swu > >>> on failure wouldn't fix the issue with missing tmp directories as swupdate > >>> would only create them on startup, this way at least if the issue gets again > >>> somehow a resend for the swu should have a decent chance of automatically > >>> fixing the issue by recreating the directories. > >> > >> Ok, let's get this in. My only fear (as in the past) is that this hides > >> the real cause (it was systemd). It is clear to me that deletion of > >> directories is not done by SWUpdate, but from an external agent. > > > > Yeah, I think this is still a good idea either way. > > > Best regards, > Stefano > > > -- > ===================================================================== > 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 > =====================================================================
On Wed, Aug 25, 2021 at 2:50 PM James Hilliard <james.hilliard1@gmail.com> wrote: > > Under some circumstances these folders may get removed after > startup, for example if /tmp has a mountpoint change after > swupdate is started. > > In order to make swupdate better able to handle these sort of > race conditions we can create/destroy these folders for each > update installation. > > Fixes: > [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > [ERROR] : SWUPDATE failed [1] Installation failed ! > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Changes v1 -> v2: > - just create/destroy datadst once for each install > --- > core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > core/swupdate.c | 57 -------------------------------------- > 2 files changed, 62 insertions(+), 66 deletions(-) > > diff --git a/core/installer.c b/core/installer.c > index 5e4cbe5..fbb5e5a 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -18,6 +18,7 @@ > #include <sys/types.h> > #include <sys/wait.h> > #include <fcntl.h> > +#include <ftw.h> > #include <sys/stat.h> > #include <sys/mount.h> > > @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > return 0; > } > > +static void create_directory(const char* path) { > + char* dpath; > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > + ENOMEM_ASPRINTF) { > + ERROR("OOM: Directory %s not created", path); > + return; > + } > + if (mkdir(dpath, 0777)) { > + WARN("Directory %s cannot be created due to : %s", > + path, strerror(errno)); > + } > + free(dpath); > +} > + > +#ifndef CONFIG_NOCLEANUP > +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > + int typeflag, struct FTW *ftwbuf) > +{ > + (void)sb; > + (void)typeflag; > + (void)ftwbuf; > + return remove(fpath); So I just ran into an issue where this can fail to work correctly if say a failure happens to occur after the archive handler has mounted the extraction directory but before the archive handler unmounts the extraction directory. This resulted in repeated update failures when swupdate attempted to retry installation with /tmp/datadst still mounted by the previous archive handler failure as I have a diskformat handler which runs prior to archive extraction that fails to format the disk as /tmp/datadst was left mounted. Failure due to /tmp/datadst being left mounted after a transient failure: SWUPDATE failed [0] ERROR diskformat.c : diskformat_mkfs : 113 : creating ext4 file system on /dev/disk/by-partlabel/main failed. -14 It's not entirely clear what the best way to handle this issue is, I'm thinking maybe we should have _remove_directory_cb check if /tmp/datadst is mounted and unmount /tmp/datadst automatically but I'm not sure if that may potentially cause secondary issues. > +} > + > +static int remove_directory(const char* path) > +{ > + char* dpath; > + int ret; > + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > + ENOMEM_ASPRINTF) { > + ERROR("OOM: Directory %s not removed", path); > + return -ENOMEM; > + } > + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > + free(dpath); > + return ret; > +} > +#endif > + > int install_single_image(struct img_type *img, bool dry_run) > { > struct installer_handler *hnd; > @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > bool dry_run = sw->parms.dry_run; > bool dropimg; > > + /* Create directories for scripts/datadst */ > + create_directory(SCRIPTS_DIR_SUFFIX); > + create_directory(DATADST_DIR_SUFFIX); > + > /* Extract all scripts, preinstall scripts must be run now */ > const char* tmpdir_scripts = get_tmpdirscripts(); > ret = extract_scripts(&sw->scripts); > ret |= extract_scripts(&sw->bootscripts); > if (ret) { > ERROR("extracting script to %s failed", tmpdir_scripts); > - return ret; > + goto out; > } > > /* Scripts must be run before installing images */ > @@ -272,7 +316,7 @@ int install_images(struct swupdate_cfg *sw) > ret = run_prepost_scripts(&sw->scripts, PREINSTALL); > if (ret) { > ERROR("execute preinstall scripts failed"); > - return ret; > + goto out; > } > } > > @@ -293,14 +337,16 @@ int install_images(struct swupdate_cfg *sw) > if (asprintf(&filename, "%s%s", TMPDIR, img->fname) == > ENOMEM_ASPRINTF) { > ERROR("Path too long: %s%s", TMPDIR, img->fname); > - return -1; > + ret = -1; > + goto out; > } > > ret = stat(filename, &buf); > if (ret) { > TRACE("%s not found or wrong", filename); > free(filename); > - return -1; > + ret = -1; > + goto out; > } > img->size = buf.st_size; > img->fdin = open(filename, O_RDONLY); > @@ -308,7 +354,8 @@ int install_images(struct swupdate_cfg *sw) > if (img->fdin < 0) { > ERROR("Image %s cannot be opened", > img->fname); > - return -1; > + ret = -1; > + goto out; > } > > if ((strlen(img->path) > 0) && > @@ -337,20 +384,20 @@ int install_images(struct swupdate_cfg *sw) > free_image(img); > > if (ret) > - return ret; > + goto out; > } > > /* > * Skip scripts in dry-run mode > */ > if (dry_run) { > - return ret; > + goto out; > } > > ret = run_prepost_scripts(&sw->scripts, POSTINSTALL); > if (ret) { > ERROR("execute postinstall scripts failed"); > - return ret; > + goto out; > } > > if (!LIST_EMPTY(&sw->bootloader)) { > @@ -358,12 +405,18 @@ int install_images(struct swupdate_cfg *sw) > sprintf(bootscript, "%s%s", TMPDIR, BOOT_SCRIPT_SUFFIX); > ret = update_bootloader_env(sw, bootscript); > if (ret) { > - return ret; > + goto out; > } > } > > ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL); > > +out: > +#ifndef CONFIG_NOCLEANUP > + remove_directory(SCRIPTS_DIR_SUFFIX); > + remove_directory(DATADST_DIR_SUFFIX); > +#endif > + > return ret; > } > > diff --git a/core/swupdate.c b/core/swupdate.c > index 949a647..a31aeb1 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -23,7 +23,6 @@ > #include <pthread.h> > #include <signal.h> > #include <sys/wait.h> > -#include <ftw.h> > > #include "bsdqueue.h" > #include "cpiohdr.h" > @@ -244,53 +243,6 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) > return 0; > } > > -static void create_directory(const char* path) { > - char* dpath; > - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > - ENOMEM_ASPRINTF) { > - ERROR("OOM: Directory %s not created", path); > - return; > - } > - if (mkdir(dpath, 0777)) { > - WARN("Directory %s cannot be created due to : %s", > - path, strerror(errno)); > - } > - free(dpath); > -} > - > -#ifndef CONFIG_NOCLEANUP > -static int _remove_directory_cb(const char *fpath, const struct stat *sb, > - int typeflag, struct FTW *ftwbuf) > -{ > - (void)sb; > - (void)typeflag; > - (void)ftwbuf; > - return remove(fpath); > -} > - > -static int remove_directory(const char* path) > -{ > - char* dpath; > - int ret; > - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > - ENOMEM_ASPRINTF) { > - ERROR("OOM: Directory %s not removed", path); > - return -ENOMEM; > - } > - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > - free(dpath); > - return ret; > -} > -#endif > - > -static void swupdate_cleanup(void) > -{ > -#ifndef CONFIG_NOCLEANUP > - remove_directory(SCRIPTS_DIR_SUFFIX); > - remove_directory(DATADST_DIR_SUFFIX); > -#endif > -} > - > static void swupdate_init(struct swupdate_cfg *sw) > { > /* Initialize internal tree to store configuration */ > @@ -303,15 +255,6 @@ static void swupdate_init(struct swupdate_cfg *sw) > LIST_INIT(&sw->extprocs); > sw->cert_purpose = SSL_PURPOSE_DEFAULT; > > - > - /* Create directories for scripts */ > - create_directory(SCRIPTS_DIR_SUFFIX); > - create_directory(DATADST_DIR_SUFFIX); > - > - if (atexit(swupdate_cleanup) != 0) { > - TRACE("Cannot setup SWUpdate cleanup on exit"); > - } > - > #ifdef CONFIG_MTD > mtd_init(); > ubi_init(); > -- > 2.32.0 >
Hi James, On 28.06.24 17:25, James Hilliard wrote: > On Wed, Aug 25, 2021 at 2:50 PM James Hilliard > <james.hilliard1@gmail.com> wrote: >> >> Under some circumstances these folders may get removed after >> startup, for example if /tmp has a mountpoint change after >> swupdate is started. >> >> In order to make swupdate better able to handle these sort of >> race conditions we can create/destroy these folders for each >> update installation. >> >> Fixes: >> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 >> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed >> [ERROR] : SWUPDATE failed [1] Installation failed ! >> >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> --- >> Changes v1 -> v2: >> - just create/destroy datadst once for each install >> --- >> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ >> core/swupdate.c | 57 -------------------------------------- >> 2 files changed, 62 insertions(+), 66 deletions(-) >> >> diff --git a/core/installer.c b/core/installer.c >> index 5e4cbe5..fbb5e5a 100644 >> --- a/core/installer.c >> +++ b/core/installer.c >> @@ -18,6 +18,7 @@ >> #include <sys/types.h> >> #include <sys/wait.h> >> #include <fcntl.h> >> +#include <ftw.h> >> #include <sys/stat.h> >> #include <sys/mount.h> >> >> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) >> return 0; >> } >> >> +static void create_directory(const char* path) { >> + char* dpath; >> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >> + ENOMEM_ASPRINTF) { >> + ERROR("OOM: Directory %s not created", path); >> + return; >> + } >> + if (mkdir(dpath, 0777)) { >> + WARN("Directory %s cannot be created due to : %s", >> + path, strerror(errno)); >> + } >> + free(dpath); >> +} >> + >> +#ifndef CONFIG_NOCLEANUP >> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, >> + int typeflag, struct FTW *ftwbuf) >> +{ >> + (void)sb; >> + (void)typeflag; >> + (void)ftwbuf; >> + return remove(fpath); > > So I just ran into an issue where this can fail to work correctly if say > a failure happens to occur after the archive handler has mounted the > extraction directory but before the archive handler unmounts the > extraction directory. This resulted in repeated update failures when > swupdate attempted to retry installation with /tmp/datadst still mounted > by the previous archive handler failure as I have a diskformat handler > which runs prior to archive extraction that fails to format the disk as > /tmp/datadst was left mounted. > > Failure due to /tmp/datadst being left mounted after a transient failure: > SWUPDATE failed [0] ERROR diskformat.c : diskformat_mkfs : 113 : > creating ext4 file system on /dev/disk/by-partlabel/main failed. -14 > I see. As failure you mean something outside SWUpdate ? If an error is detected by the archive handler, it should free all resources, including unmounting. To force this case, SWUpdate should be killed in some way, and then I can imagine the case you are describing. Is it this the case ? > It's not entirely clear what the best way to handle this issue is, I'm thinking > maybe we should have _remove_directory_cb check if /tmp/datadst is > mounted and unmount /tmp/datadst automatically but I'm not sure if that > may potentially cause secondary issues. Let't check : the directory live for an a single update and check for creation is done before the update is started (call of swupdate_create_directory). As it is supposed that directories couldn't exist, it is also a safe place to check if a mount point is set to the directory, and force a remount. But even you r proposal is good: we want to remove directories, it is supposed that there is no mount point to these directories. IMHO I do not see drawbacks. Best regards. Stefano > >> +} >> + >> +static int remove_directory(const char* path) >> +{ >> + char* dpath; >> + int ret; >> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >> + ENOMEM_ASPRINTF) { >> + ERROR("OOM: Directory %s not removed", path); >> + return -ENOMEM; >> + } >> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >> + free(dpath); >> + return ret; >> +} >> +#endif >> + >> int install_single_image(struct img_type *img, bool dry_run) >> { >> struct installer_handler *hnd; >> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) >> bool dry_run = sw->parms.dry_run; >> bool dropimg; >> >> + /* Create directories for scripts/datadst */ >> + create_directory(SCRIPTS_DIR_SUFFIX); >> + create_directory(DATADST_DIR_SUFFIX); >> + >> /* Extract all scripts, preinstall scripts must be run now */ >> const char* tmpdir_scripts = get_tmpdirscripts(); >> ret = extract_scripts(&sw->scripts); >> ret |= extract_scripts(&sw->bootscripts); >> if (ret) { >> ERROR("extracting script to %s failed", tmpdir_scripts); >> - return ret; >> + goto out; >> } >> >> /* Scripts must be run before installing images */ >> @@ -272,7 +316,7 @@ int install_images(struct swupdate_cfg *sw) >> ret = run_prepost_scripts(&sw->scripts, PREINSTALL); >> if (ret) { >> ERROR("execute preinstall scripts failed"); >> - return ret; >> + goto out; >> } >> } >> >> @@ -293,14 +337,16 @@ int install_images(struct swupdate_cfg *sw) >> if (asprintf(&filename, "%s%s", TMPDIR, img->fname) == >> ENOMEM_ASPRINTF) { >> ERROR("Path too long: %s%s", TMPDIR, img->fname); >> - return -1; >> + ret = -1; >> + goto out; >> } >> >> ret = stat(filename, &buf); >> if (ret) { >> TRACE("%s not found or wrong", filename); >> free(filename); >> - return -1; >> + ret = -1; >> + goto out; >> } >> img->size = buf.st_size; >> img->fdin = open(filename, O_RDONLY); >> @@ -308,7 +354,8 @@ int install_images(struct swupdate_cfg *sw) >> if (img->fdin < 0) { >> ERROR("Image %s cannot be opened", >> img->fname); >> - return -1; >> + ret = -1; >> + goto out; >> } >> >> if ((strlen(img->path) > 0) && >> @@ -337,20 +384,20 @@ int install_images(struct swupdate_cfg *sw) >> free_image(img); >> >> if (ret) >> - return ret; >> + goto out; >> } >> >> /* >> * Skip scripts in dry-run mode >> */ >> if (dry_run) { >> - return ret; >> + goto out; >> } >> >> ret = run_prepost_scripts(&sw->scripts, POSTINSTALL); >> if (ret) { >> ERROR("execute postinstall scripts failed"); >> - return ret; >> + goto out; >> } >> >> if (!LIST_EMPTY(&sw->bootloader)) { >> @@ -358,12 +405,18 @@ int install_images(struct swupdate_cfg *sw) >> sprintf(bootscript, "%s%s", TMPDIR, BOOT_SCRIPT_SUFFIX); >> ret = update_bootloader_env(sw, bootscript); >> if (ret) { >> - return ret; >> + goto out; >> } >> } >> >> ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL); >> >> +out: >> +#ifndef CONFIG_NOCLEANUP >> + remove_directory(SCRIPTS_DIR_SUFFIX); >> + remove_directory(DATADST_DIR_SUFFIX); >> +#endif >> + >> return ret; >> } >> >> diff --git a/core/swupdate.c b/core/swupdate.c >> index 949a647..a31aeb1 100644 >> --- a/core/swupdate.c >> +++ b/core/swupdate.c >> @@ -23,7 +23,6 @@ >> #include <pthread.h> >> #include <signal.h> >> #include <sys/wait.h> >> -#include <ftw.h> >> >> #include "bsdqueue.h" >> #include "cpiohdr.h" >> @@ -244,53 +243,6 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) >> return 0; >> } >> >> -static void create_directory(const char* path) { >> - char* dpath; >> - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >> - ENOMEM_ASPRINTF) { >> - ERROR("OOM: Directory %s not created", path); >> - return; >> - } >> - if (mkdir(dpath, 0777)) { >> - WARN("Directory %s cannot be created due to : %s", >> - path, strerror(errno)); >> - } >> - free(dpath); >> -} >> - >> -#ifndef CONFIG_NOCLEANUP >> -static int _remove_directory_cb(const char *fpath, const struct stat *sb, >> - int typeflag, struct FTW *ftwbuf) >> -{ >> - (void)sb; >> - (void)typeflag; >> - (void)ftwbuf; >> - return remove(fpath); >> -} >> - >> -static int remove_directory(const char* path) >> -{ >> - char* dpath; >> - int ret; >> - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == >> - ENOMEM_ASPRINTF) { >> - ERROR("OOM: Directory %s not removed", path); >> - return -ENOMEM; >> - } >> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >> - free(dpath); >> - return ret; >> -} >> -#endif >> - >> -static void swupdate_cleanup(void) >> -{ >> -#ifndef CONFIG_NOCLEANUP >> - remove_directory(SCRIPTS_DIR_SUFFIX); >> - remove_directory(DATADST_DIR_SUFFIX); >> -#endif >> -} >> - >> static void swupdate_init(struct swupdate_cfg *sw) >> { >> /* Initialize internal tree to store configuration */ >> @@ -303,15 +255,6 @@ static void swupdate_init(struct swupdate_cfg *sw) >> LIST_INIT(&sw->extprocs); >> sw->cert_purpose = SSL_PURPOSE_DEFAULT; >> >> - >> - /* Create directories for scripts */ >> - create_directory(SCRIPTS_DIR_SUFFIX); >> - create_directory(DATADST_DIR_SUFFIX); >> - >> - if (atexit(swupdate_cleanup) != 0) { >> - TRACE("Cannot setup SWUpdate cleanup on exit"); >> - } >> - >> #ifdef CONFIG_MTD >> mtd_init(); >> ubi_init(); >> -- >> 2.32.0 >> >
On Sat, Jun 29, 2024 at 4:07 AM Stefano Babic <stefano.babic@swupdate.org> wrote: > > Hi James, > > On 28.06.24 17:25, James Hilliard wrote: > > On Wed, Aug 25, 2021 at 2:50 PM James Hilliard > > <james.hilliard1@gmail.com> wrote: > >> > >> Under some circumstances these folders may get removed after > >> startup, for example if /tmp has a mountpoint change after > >> swupdate is started. > >> > >> In order to make swupdate better able to handle these sort of > >> race conditions we can create/destroy these folders for each > >> update installation. > >> > >> Fixes: > >> [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 > >> [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed > >> [ERROR] : SWUPDATE failed [1] Installation failed ! > >> > >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >> --- > >> Changes v1 -> v2: > >> - just create/destroy datadst once for each install > >> --- > >> core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ > >> core/swupdate.c | 57 -------------------------------------- > >> 2 files changed, 62 insertions(+), 66 deletions(-) > >> > >> diff --git a/core/installer.c b/core/installer.c > >> index 5e4cbe5..fbb5e5a 100644 > >> --- a/core/installer.c > >> +++ b/core/installer.c > >> @@ -18,6 +18,7 @@ > >> #include <sys/types.h> > >> #include <sys/wait.h> > >> #include <fcntl.h> > >> +#include <ftw.h> > >> #include <sys/stat.h> > >> #include <sys/mount.h> > >> > >> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > >> return 0; > >> } > >> > >> +static void create_directory(const char* path) { > >> + char* dpath; > >> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >> + ENOMEM_ASPRINTF) { > >> + ERROR("OOM: Directory %s not created", path); > >> + return; > >> + } > >> + if (mkdir(dpath, 0777)) { > >> + WARN("Directory %s cannot be created due to : %s", > >> + path, strerror(errno)); > >> + } > >> + free(dpath); > >> +} > >> + > >> +#ifndef CONFIG_NOCLEANUP > >> +static int _remove_directory_cb(const char *fpath, const struct stat *sb, > >> + int typeflag, struct FTW *ftwbuf) > >> +{ > >> + (void)sb; > >> + (void)typeflag; > >> + (void)ftwbuf; > >> + return remove(fpath); > > > > So I just ran into an issue where this can fail to work correctly if say > > a failure happens to occur after the archive handler has mounted the > > extraction directory but before the archive handler unmounts the > > extraction directory. This resulted in repeated update failures when > > swupdate attempted to retry installation with /tmp/datadst still mounted > > by the previous archive handler failure as I have a diskformat handler > > which runs prior to archive extraction that fails to format the disk as > > /tmp/datadst was left mounted. > > > > Failure due to /tmp/datadst being left mounted after a transient failure: > > SWUPDATE failed [0] ERROR diskformat.c : diskformat_mkfs : 113 : > > creating ext4 file system on /dev/disk/by-partlabel/main failed. -14 > > > > I see. > > As failure you mean something outside SWUpdate ? If an error is detected > by the archive handler, it should free all resources, including > unmounting. To force this case, SWUpdate should be killed in some way, > and then I can imagine the case you are describing. Is it this the case ? Presumably this was due to a crash/failure inside the archive handler which resulted in the extraction path not getting unmounted, we didn't have logs of the original failure due to them being overwritten but we run swupdate inside of systemd so if swupdate crashed AFAIU it would have been automatically restarted. We first noticed the issue when debugging a report of excessive data usage which was determined to be caused by swupdate repeatedly downloading and failing to install an update due to datadst being mounted to the archive handler extraction target directory. > > > It's not entirely clear what the best way to handle this issue is, I'm thinking > > maybe we should have _remove_directory_cb check if /tmp/datadst is > > mounted and unmount /tmp/datadst automatically but I'm not sure if that > > may potentially cause secondary issues. > > Let't check : the directory live for an a single update and check for > creation is done before the update is started (call of > swupdate_create_directory). As it is supposed that directories couldn't > exist, it is also a safe place to check if a mount point is set to the > directory, and force a remount. But even you r proposal is good: we want > to remove directories, it is supposed that there is no mount point to > these directories. IMHO I do not see drawbacks. Yeah, so I think for safety we should make sure we do the mountpoint check before trying to remove the directory. From my understanding otherwise we may accidentally delete files in the mounted directory which could leave the system in a bad state. Something along these lines maybe: https://patchwork.ozlabs.org/project/swupdate/patch/20240629224706.1164950-1-james.hilliard1@gmail.com/ > > Best regards. > Stefano > > > > > >> +} > >> + > >> +static int remove_directory(const char* path) > >> +{ > >> + char* dpath; > >> + int ret; > >> + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >> + ENOMEM_ASPRINTF) { > >> + ERROR("OOM: Directory %s not removed", path); > >> + return -ENOMEM; > >> + } > >> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > >> + free(dpath); > >> + return ret; > >> +} > >> +#endif > >> + > >> int install_single_image(struct img_type *img, bool dry_run) > >> { > >> struct installer_handler *hnd; > >> @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) > >> bool dry_run = sw->parms.dry_run; > >> bool dropimg; > >> > >> + /* Create directories for scripts/datadst */ > >> + create_directory(SCRIPTS_DIR_SUFFIX); > >> + create_directory(DATADST_DIR_SUFFIX); > >> + > >> /* Extract all scripts, preinstall scripts must be run now */ > >> const char* tmpdir_scripts = get_tmpdirscripts(); > >> ret = extract_scripts(&sw->scripts); > >> ret |= extract_scripts(&sw->bootscripts); > >> if (ret) { > >> ERROR("extracting script to %s failed", tmpdir_scripts); > >> - return ret; > >> + goto out; > >> } > >> > >> /* Scripts must be run before installing images */ > >> @@ -272,7 +316,7 @@ int install_images(struct swupdate_cfg *sw) > >> ret = run_prepost_scripts(&sw->scripts, PREINSTALL); > >> if (ret) { > >> ERROR("execute preinstall scripts failed"); > >> - return ret; > >> + goto out; > >> } > >> } > >> > >> @@ -293,14 +337,16 @@ int install_images(struct swupdate_cfg *sw) > >> if (asprintf(&filename, "%s%s", TMPDIR, img->fname) == > >> ENOMEM_ASPRINTF) { > >> ERROR("Path too long: %s%s", TMPDIR, img->fname); > >> - return -1; > >> + ret = -1; > >> + goto out; > >> } > >> > >> ret = stat(filename, &buf); > >> if (ret) { > >> TRACE("%s not found or wrong", filename); > >> free(filename); > >> - return -1; > >> + ret = -1; > >> + goto out; > >> } > >> img->size = buf.st_size; > >> img->fdin = open(filename, O_RDONLY); > >> @@ -308,7 +354,8 @@ int install_images(struct swupdate_cfg *sw) > >> if (img->fdin < 0) { > >> ERROR("Image %s cannot be opened", > >> img->fname); > >> - return -1; > >> + ret = -1; > >> + goto out; > >> } > >> > >> if ((strlen(img->path) > 0) && > >> @@ -337,20 +384,20 @@ int install_images(struct swupdate_cfg *sw) > >> free_image(img); > >> > >> if (ret) > >> - return ret; > >> + goto out; > >> } > >> > >> /* > >> * Skip scripts in dry-run mode > >> */ > >> if (dry_run) { > >> - return ret; > >> + goto out; > >> } > >> > >> ret = run_prepost_scripts(&sw->scripts, POSTINSTALL); > >> if (ret) { > >> ERROR("execute postinstall scripts failed"); > >> - return ret; > >> + goto out; > >> } > >> > >> if (!LIST_EMPTY(&sw->bootloader)) { > >> @@ -358,12 +405,18 @@ int install_images(struct swupdate_cfg *sw) > >> sprintf(bootscript, "%s%s", TMPDIR, BOOT_SCRIPT_SUFFIX); > >> ret = update_bootloader_env(sw, bootscript); > >> if (ret) { > >> - return ret; > >> + goto out; > >> } > >> } > >> > >> ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL); > >> > >> +out: > >> +#ifndef CONFIG_NOCLEANUP > >> + remove_directory(SCRIPTS_DIR_SUFFIX); > >> + remove_directory(DATADST_DIR_SUFFIX); > >> +#endif > >> + > >> return ret; > >> } > >> > >> diff --git a/core/swupdate.c b/core/swupdate.c > >> index 949a647..a31aeb1 100644 > >> --- a/core/swupdate.c > >> +++ b/core/swupdate.c > >> @@ -23,7 +23,6 @@ > >> #include <pthread.h> > >> #include <signal.h> > >> #include <sys/wait.h> > >> -#include <ftw.h> > >> > >> #include "bsdqueue.h" > >> #include "cpiohdr.h" > >> @@ -244,53 +243,6 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) > >> return 0; > >> } > >> > >> -static void create_directory(const char* path) { > >> - char* dpath; > >> - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >> - ENOMEM_ASPRINTF) { > >> - ERROR("OOM: Directory %s not created", path); > >> - return; > >> - } > >> - if (mkdir(dpath, 0777)) { > >> - WARN("Directory %s cannot be created due to : %s", > >> - path, strerror(errno)); > >> - } > >> - free(dpath); > >> -} > >> - > >> -#ifndef CONFIG_NOCLEANUP > >> -static int _remove_directory_cb(const char *fpath, const struct stat *sb, > >> - int typeflag, struct FTW *ftwbuf) > >> -{ > >> - (void)sb; > >> - (void)typeflag; > >> - (void)ftwbuf; > >> - return remove(fpath); > >> -} > >> - > >> -static int remove_directory(const char* path) > >> -{ > >> - char* dpath; > >> - int ret; > >> - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == > >> - ENOMEM_ASPRINTF) { > >> - ERROR("OOM: Directory %s not removed", path); > >> - return -ENOMEM; > >> - } > >> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > >> - free(dpath); > >> - return ret; > >> -} > >> -#endif > >> - > >> -static void swupdate_cleanup(void) > >> -{ > >> -#ifndef CONFIG_NOCLEANUP > >> - remove_directory(SCRIPTS_DIR_SUFFIX); > >> - remove_directory(DATADST_DIR_SUFFIX); > >> -#endif > >> -} > >> - > >> static void swupdate_init(struct swupdate_cfg *sw) > >> { > >> /* Initialize internal tree to store configuration */ > >> @@ -303,15 +255,6 @@ static void swupdate_init(struct swupdate_cfg *sw) > >> LIST_INIT(&sw->extprocs); > >> sw->cert_purpose = SSL_PURPOSE_DEFAULT; > >> > >> - > >> - /* Create directories for scripts */ > >> - create_directory(SCRIPTS_DIR_SUFFIX); > >> - create_directory(DATADST_DIR_SUFFIX); > >> - > >> - if (atexit(swupdate_cleanup) != 0) { > >> - TRACE("Cannot setup SWUpdate cleanup on exit"); > >> - } > >> - > >> #ifdef CONFIG_MTD > >> mtd_init(); > >> ubi_init(); > >> -- > >> 2.32.0 > >> > > >
diff --git a/core/installer.c b/core/installer.c index 5e4cbe5..fbb5e5a 100644 --- a/core/installer.c +++ b/core/installer.c @@ -18,6 +18,7 @@ #include <sys/types.h> #include <sys/wait.h> #include <fcntl.h> +#include <ftw.h> #include <sys/stat.h> #include <sys/mount.h> @@ -209,6 +210,45 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) return 0; } +static void create_directory(const char* path) { + char* dpath; + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == + ENOMEM_ASPRINTF) { + ERROR("OOM: Directory %s not created", path); + return; + } + if (mkdir(dpath, 0777)) { + WARN("Directory %s cannot be created due to : %s", + path, strerror(errno)); + } + free(dpath); +} + +#ifndef CONFIG_NOCLEANUP +static int _remove_directory_cb(const char *fpath, const struct stat *sb, + int typeflag, struct FTW *ftwbuf) +{ + (void)sb; + (void)typeflag; + (void)ftwbuf; + return remove(fpath); +} + +static int remove_directory(const char* path) +{ + char* dpath; + int ret; + if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == + ENOMEM_ASPRINTF) { + ERROR("OOM: Directory %s not removed", path); + return -ENOMEM; + } + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); + free(dpath); + return ret; +} +#endif + int install_single_image(struct img_type *img, bool dry_run) { struct installer_handler *hnd; @@ -258,13 +298,17 @@ int install_images(struct swupdate_cfg *sw) bool dry_run = sw->parms.dry_run; bool dropimg; + /* Create directories for scripts/datadst */ + create_directory(SCRIPTS_DIR_SUFFIX); + create_directory(DATADST_DIR_SUFFIX); + /* Extract all scripts, preinstall scripts must be run now */ const char* tmpdir_scripts = get_tmpdirscripts(); ret = extract_scripts(&sw->scripts); ret |= extract_scripts(&sw->bootscripts); if (ret) { ERROR("extracting script to %s failed", tmpdir_scripts); - return ret; + goto out; } /* Scripts must be run before installing images */ @@ -272,7 +316,7 @@ int install_images(struct swupdate_cfg *sw) ret = run_prepost_scripts(&sw->scripts, PREINSTALL); if (ret) { ERROR("execute preinstall scripts failed"); - return ret; + goto out; } } @@ -293,14 +337,16 @@ int install_images(struct swupdate_cfg *sw) if (asprintf(&filename, "%s%s", TMPDIR, img->fname) == ENOMEM_ASPRINTF) { ERROR("Path too long: %s%s", TMPDIR, img->fname); - return -1; + ret = -1; + goto out; } ret = stat(filename, &buf); if (ret) { TRACE("%s not found or wrong", filename); free(filename); - return -1; + ret = -1; + goto out; } img->size = buf.st_size; img->fdin = open(filename, O_RDONLY); @@ -308,7 +354,8 @@ int install_images(struct swupdate_cfg *sw) if (img->fdin < 0) { ERROR("Image %s cannot be opened", img->fname); - return -1; + ret = -1; + goto out; } if ((strlen(img->path) > 0) && @@ -337,20 +384,20 @@ int install_images(struct swupdate_cfg *sw) free_image(img); if (ret) - return ret; + goto out; } /* * Skip scripts in dry-run mode */ if (dry_run) { - return ret; + goto out; } ret = run_prepost_scripts(&sw->scripts, POSTINSTALL); if (ret) { ERROR("execute postinstall scripts failed"); - return ret; + goto out; } if (!LIST_EMPTY(&sw->bootloader)) { @@ -358,12 +405,18 @@ int install_images(struct swupdate_cfg *sw) sprintf(bootscript, "%s%s", TMPDIR, BOOT_SCRIPT_SUFFIX); ret = update_bootloader_env(sw, bootscript); if (ret) { - return ret; + goto out; } } ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL); +out: +#ifndef CONFIG_NOCLEANUP + remove_directory(SCRIPTS_DIR_SUFFIX); + remove_directory(DATADST_DIR_SUFFIX); +#endif + return ret; } diff --git a/core/swupdate.c b/core/swupdate.c index 949a647..a31aeb1 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -23,7 +23,6 @@ #include <pthread.h> #include <signal.h> #include <sys/wait.h> -#include <ftw.h> #include "bsdqueue.h" #include "cpiohdr.h" @@ -244,53 +243,6 @@ static int parse_image_selector(const char *selector, struct swupdate_cfg *sw) return 0; } -static void create_directory(const char* path) { - char* dpath; - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == - ENOMEM_ASPRINTF) { - ERROR("OOM: Directory %s not created", path); - return; - } - if (mkdir(dpath, 0777)) { - WARN("Directory %s cannot be created due to : %s", - path, strerror(errno)); - } - free(dpath); -} - -#ifndef CONFIG_NOCLEANUP -static int _remove_directory_cb(const char *fpath, const struct stat *sb, - int typeflag, struct FTW *ftwbuf) -{ - (void)sb; - (void)typeflag; - (void)ftwbuf; - return remove(fpath); -} - -static int remove_directory(const char* path) -{ - char* dpath; - int ret; - if (asprintf(&dpath, "%s%s", get_tmpdir(), path) == - ENOMEM_ASPRINTF) { - ERROR("OOM: Directory %s not removed", path); - return -ENOMEM; - } - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); - free(dpath); - return ret; -} -#endif - -static void swupdate_cleanup(void) -{ -#ifndef CONFIG_NOCLEANUP - remove_directory(SCRIPTS_DIR_SUFFIX); - remove_directory(DATADST_DIR_SUFFIX); -#endif -} - static void swupdate_init(struct swupdate_cfg *sw) { /* Initialize internal tree to store configuration */ @@ -303,15 +255,6 @@ static void swupdate_init(struct swupdate_cfg *sw) LIST_INIT(&sw->extprocs); sw->cert_purpose = SSL_PURPOSE_DEFAULT; - - /* Create directories for scripts */ - create_directory(SCRIPTS_DIR_SUFFIX); - create_directory(DATADST_DIR_SUFFIX); - - if (atexit(swupdate_cleanup) != 0) { - TRACE("Cannot setup SWUpdate cleanup on exit"); - } - #ifdef CONFIG_MTD mtd_init(); ubi_init();
Under some circumstances these folders may get removed after startup, for example if /tmp has a mountpoint change after swupdate is started. In order to make swupdate better able to handle these sort of race conditions we can create/destroy these folders for each update installation. Fixes: [ERROR] : SWUPDATE failed [0] ERROR : I cannot open /tmp/scripts/format.lua 2 [ERROR] : SWUPDATE failed [0] ERROR : extracting script to /tmp/scripts/ failed [ERROR] : SWUPDATE failed [1] Installation failed ! Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- Changes v1 -> v2: - just create/destroy datadst once for each install --- core/installer.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ core/swupdate.c | 57 -------------------------------------- 2 files changed, 62 insertions(+), 66 deletions(-)