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.faine...@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
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
