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.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.
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
--
Florian
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
