Re: [PATCH iproute2 v2 1/2] bpf: Fix segfault when custom pinning is used

[Dominique Martinet <asmadeus@codewreck.org>]

Andrea Claudi wrote on Wed, Apr 22, 2020:
> > -       ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> > +       ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> 
> Shouldn't we check for the last argument length before? We should have
> enough space for "/../" and the terminator, so we need the last
> argument length to be less than PATH_MAX-5, right?

snprintf will return the length that would be written if there had been
room so most codes just check the return value instead (something like
if (ret >= sizeof(tmp)) error)

OTOH this will actually be caught by the later strcat guard, because rem
will always contain at least on / the while will always be entered and
strlen(tmp) + (>=0) + 2 will always be > PATH_MAX, so this function will
error out.
I'll admit it's not clear, though :)

I'm actually not sure snprintf can return < 0... wide character
formatting functions can but basic ones not really, there's hardly any
snprintf return checking in iproute2 code...


Anyway, with all that said this patch currently technically works for
me, despite being not so clear, so can add my reviewed-by on whatever
final version you take (check < 0 or >= PATH_MAX or none or both), if
you'd like :)

-- 
Dominique