[PATCH] state: check return value of chown

[Karl Palsson]

Daniel Golle <daniel at makrotopia.org> wrote:
> On Mon, Jan 04, 2021 at 05:08:22PM -0000, Karl Palsson wrote:
> > 
> > Daniel Golle <daniel at makrotopia.org> wrote:
> > > On Sat, Jan 02, 2021 at 10:01:36PM +0800, yeholmes at outlook.com
> > > wrote:
> > 
> > > > -			chown(p->pw_dir, p->pw_uid, p->pw_gid);
> > > > +			if (chown(p->pw_dir, p->pw_uid, p->pw_gid))
> > > > +				fprintf(stderr, "Failed to change ownership for %s\n", p->pw_dir);
> > > 
> > > Please let's not have a custom error message for cases which
> > > practically never occur. If we would really cover all that,
> > > around 80% of the size of executables like procd would be error
> > > messages. Imho an assertion is the right thing to do here.
> > > 
> > 
> > Do we compile with assertions enabled?
> 
> Interesting point actually: I just checked, assertions are
> compiled into code unless NDEBUG is defined which we don't.
> Could be an idea for tiny targets to do so...

That... might make things worse :) For instance, in the patch as
it is proposed now...

-			chown(p->pw_dir, p->pw_uid, p->pw_gid);
+			assert(chown(p->pw_dir, p->pw_uid, p->pw_gid) == 0);

the chown will never get called if you compile with NDEBUG....
eg, (We want to get 2 printed in every case, this is when the
asserts pass)


karlp at beros:~/src/junk$ cat hate.c 
#include <assert.h>
#include <stdio.h>

static int k;

void call_it_safe(void) {
	assert(++k > 0);
}

void yolo(void) {
	k++;
}

int main() {
	call_it_safe();
	yolo();
	printf("y0: %d\n", k);
}
karlp at beros:~/src/junk$ cc hate.c -DNDEBUG -o hate_ndebug
karlp at beros:~/src/junk$ cc hate.c -o hate karlp at beros:~/src/junk$
./hate y0: 2 << good karlp at beros:~/src/junk$ ./hate_ndebug y0: 1
<<< oops! karlp at beros:~/src/junk$

This is akin to the classic problem of logging macros turning off
code. You need to introduce an extra variable for the assert to
check. (If you want to keep going down this path)

Sincerely,
Karl Palsson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP-digital-signature.html
Type: application/pgp-signature
Size: 1175 bytes
Desc: OpenPGP Digital Signature
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20210104/ad309c33/attachment.sig>