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

[Florian Fainelli]

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

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