[PATCH v2 1/1] Create and destroy scripts/datadst temp folders on install.

[James Hilliard]

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.h...@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);
+}
+
+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

[Stefano Babic]

Hi James,

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).

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).

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: sba...@denx.de
=====================================================================

[Christian Storm]

Hi,

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

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[James Hilliard]

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.h...@gmail.com>
---

Changes v2 -> v3:
- move create_directory/remove_directory functions to util
- move create/remove directory calls to network_initializer

Changes v1 -> v2:
- just create/destroy datadst once for each install
---

core/stream_interface.c | 9 +++++++
core/swupdate.c | 57 -----------------------------------------
core/util.c | 40 +++++++++++++++++++++++++++++
include/util.h | 5 ++++
4 files changed, 54 insertions(+), 57 deletions(-)

diff --git a/core/stream_interface.c b/core/stream_interface.c
index da0c733..89525a5 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -525,6 +525,10 @@ void *network_initializer(void *data)

notify(START, RECOVERY_NO_ERROR, INFOLEVEL, "Software Update started !");

TRACE("Software update started");

+ /* Create directories for scripts/datadst */

+ swupdate_create_directory(SCRIPTS_DIR_SUFFIX);
+ swupdate_create_directory(DATADST_DIR_SUFFIX);
+
req = &inst.req;

/*
@@ -657,6 +661,11 @@ void *network_initializer(void *data)

/* release temp files we may have created */
cleanup_files(software);
+
+#ifndef CONFIG_NOCLEANUP
+ swupdate_remove_directory(SCRIPTS_DIR_SUFFIX);
+ swupdate_remove_directory(DATADST_DIR_SUFFIX);
+#endif
}

pthread_exit((void *)0);

diff --git a/core/util.c b/core/util.c
index 6188650..7e96652 100644
--- a/core/util.c
+++ b/core/util.c
@@ -12,6 +12,7 @@
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
+#include <ftw.h>
#include <sys/file.h>
#include <sys/stat.h>
#include <sys/param.h>
@@ -146,6 +147,45 @@ const char* get_tmpdirscripts(void)
return TMPDIRSCRIPT;
}

+void swupdate_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);
+}
+

+int swupdate_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
+

char **splitargs(char *args, int *argc)
{
char **argv = NULL;
diff --git a/include/util.h b/include/util.h
index 9f29f5f..a817f1c 100644
--- a/include/util.h
+++ b/include/util.h
@@ -245,6 +245,11 @@ unsigned long long ustrtoull(const char *cp, unsigned int base);
const char* get_tmpdir(void);
const char* get_tmpdirscripts(void);

+void swupdate_create_directory(const char* path);
+#ifndef CONFIG_NOCLEANUP
+int swupdate_remove_directory(const char* path);
+#endif
+
int swupdate_mount(const char *device, const char *dir, const char *fstype);
int swupdate_umount(const char *dir);

--
2.32.0

[James Hilliard]

Ok, moved those back to util.c and renamed them as suggested for v3.

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+u...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210826133052.txu6egjengdfje4f%40MD1ZFJVC.ad001.siemens.net.

[Christian Storm]

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.

[Stefano Babic]

Hi James,

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+u...@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210826133052.txu6egjengdfje4f%40MD1ZFJVC.ad001.siemens.net.
>

[James Hilliard]

Yeah, this should be sufficient for resolving the issue I'm hitting at least.

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+u...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20210827074411.ja43gjpyuq5eav7a%40MD1ZFJVC.ad001.siemens.net.

[James Hilliard]

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.

Yeah, I think this is still a good idea either way.

[Stefano Babic]

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

[Stefano Babic]

Hi James,

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.

[James Hilliard]

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.

[James Hilliard]

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.

[Stefano Babic]

Hi James,

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

[James Hilliard]

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.11649...@gmail.com/