[LEDE-DEV] [PATCH procd 1/2] init: Propagate sysupgrade_exec_upgraded() return value

[Matthias Schiffer]

On 07/15/2017 09:44 PM, Florian Fainelli wrote:
> chroot() can fail and its return value should be checked against so propagate
> sysupgrade_exec_upgraded() return value to its caller.
> 
> Fixes: 63789e51ed91 ("init: add support for sysupgrades triggered from preinit")
> Signed-off-by: Florian Fainelli <f.fainelli at gmail.com>

See comments inline.

> ---
>  system.c     |  7 +++----
>  sysupgrade.c | 10 ++++++----
>  sysupgrade.h |  2 +-
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/system.c b/system.c
> index 6cd2b624b3be..59ddc214e6f6 100644
> --- a/system.c
> +++ b/system.c
> @@ -400,10 +400,9 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
>  	if (!tb[SYSUPGRADE_PATH] || !tb[SYSUPGRADE_PREFIX])
>  		return UBUS_STATUS_INVALID_ARGUMENT;
>  
> -	sysupgrade_exec_upgraded(blobmsg_get_string(tb[SYSUPGRADE_PREFIX]),
> -				 blobmsg_get_string(tb[SYSUPGRADE_PATH]),
> -				 tb[SYSUPGRADE_COMMAND] ? blobmsg_get_string(tb[SYSUPGRADE_COMMAND]) : NULL);
> -	return 0;
> +	return sysupgrade_exec_upgraded(blobmsg_get_string(tb[SYSUPGRADE_PREFIX]),
> +					blobmsg_get_string(tb[SYSUPGRADE_PATH]),
> +					tb[SYSUPGRADE_COMMAND] ? blobmsg_get_string(tb[SYSUPGRADE_COMMAND]) : NULL);

This should return one of the UBUS_STATUS_* constants (I guess
UBUS_STATUS_UNKNOWN_ERROR is the only one that makes sense in case of
failure...)

We might even go as far as returning UBUS_STATUS_UNKNOWN_ERROR
unconditionally: either procd has successfully replaced itself with
upgraded and the ubus client will wait for a response forever (i.e. until
it is killed by stage2), or something has gone wrong.


>  }
>  
>  static void
> diff --git a/sysupgrade.c b/sysupgrade.c
> index 30f1836135c9..13ba4050527d 100644
> --- a/sysupgrade.c
> +++ b/sysupgrade.c
> @@ -22,14 +22,16 @@
>  #include <unistd.h>
>  
>  
> -void sysupgrade_exec_upgraded(const char *prefix, char *path, char *command)
> +int sysupgrade_exec_upgraded(const char *prefix, char *path, char *command)
>  {
>  	char *wdt_fd = watchdog_fd();
>  	char *argv[] = { "/sbin/upgraded", NULL, NULL, NULL};
> +	int ret;
>  
> -	if (chroot(prefix)) {
> +	ret = chroot(prefix);
> +	if (ret < 0) {
>  		fprintf(stderr, "Failed to chroot for upgraded exec.\n");
> -		return;
> +		return ret;
>  	}
>  
>  	argv[1] = path;
> @@ -45,5 +47,5 @@ void sysupgrade_exec_upgraded(const char *prefix, char *path, char *command)
>  	fprintf(stderr, "Failed to exec upgraded.\n");
>  	unsetenv("WDTFD");
>  	watchdog_set_cloexec(true);
> -	chroot(".");
> +	return chroot(".");
>  }

This chroot must not not fail, otherwise we'll have PID1 in a chroot. (And
I don't see how it could fail, given the first chroot was successful...) It
might make sense to simply print an error and exit(1) in this case to
deliberately cause a kernel panic.


> diff --git a/sysupgrade.h b/sysupgrade.h
> index 8c09fc99d191..3887f71a00a6 100644
> --- a/sysupgrade.h
> +++ b/sysupgrade.h
> @@ -15,7 +15,7 @@
>  #define __PROCD_SYSUPGRADE_H
>  
>  
> -void sysupgrade_exec_upgraded(const char *prefix, char *path, char *command);
> +int sysupgrade_exec_upgraded(const char *prefix, char *path, char *command);
>  
>  
>  #endif
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/lede-dev/attachments/20170716/1bdee9a4/attachment.sig>