| Message ID | 20170715194424.6494-1-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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@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 >
On 07/16/2017 10:39 AM, Matthias Schiffer wrote: > 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@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. Yes that makes sense, so maybe don't propagate the error but just exit(1) in case of error would be good enough, does that work for you? Thanks
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); } 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("."); } 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
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@gmail.com> --- system.c | 7 +++---- sysupgrade.c | 10 ++++++---- sysupgrade.h | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-)