On Wed, Nov 8, 2023 at 2:09 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> > On Fri, 2023-10-27 at 11:13 -0700, Andrii Nakryiko wrote:
>
> I read through whole program and it seems to be a good specimen of an
> idiomatic C program. Aside from two nitpicks below I can't really complain
> about the code.
>
> On the other hand, I'm a bit at odds with the core idea.
> The algorithm for ranges computation repeats the one used in kernel
> (arguably in a bit more elegant way). So, effectively we check if two
> implementations of the same algorithm end up with the same answers.
Yes, that was the point. It's a way to cross-check a delicate and
complicated logic that is very hard to cover through manual tests. So
generated testing seemed like a better approach, for which we have to
have an alternative implementation.
Note also that it's not really just a reimplementation. While the core
idea is the same, how we get there is quite different. Kernel
implementation doesn't have any notion of casting ranges. It also
employs and keeps in sync tnum, which selftests implementation doesn't
do.
So while they do strive to implement the same behavior, they do it in
quite a different way.
>
> It is a maintenance burden in a way.
It's unlikely to need changes, so I hope it won't be. But it's also
good to have as a cross-check for future work, like Shung-Hsi Yu's
attempt to make range tracking sign-agnostic.
If this becomes a burden to maintain, it's simple to remove the
selftest. But given it's written and debugged, it adds value and makes
verifier changes a bit less scary.
> What are the benefits of such approach?
> Simpler userspace prototyping for new range tracking features?
>
> [...]
>
> > +__printf(2, 3)
> > +static inline void snappendf(struct strbuf *s, const char *fmt, ...)
> > +{
> > + va_list args;
> > +
> > + va_start(args, fmt);
> > + s->pos += vsnprintf(s->buf + s->pos, s->buf_sz - s->pos, fmt, args);
> > + va_end(args);
> > +}
>
> The manpage for vsnprintf says the following about it's return value:
>
> ... If the output was truncated due to this limit, then the return
> value is the number of characters (excluding the terminating null
> byte) which would have been written to the final string if enough
> space had been available ...
>
> Which is definitely a footgun to say the least. So, I picked strbuf,
> DEFINE_STRBUF, snappendf definitions to a separate file and tried the
> following [0]:
>
> $ cat test.c
> ...
> int main(int argc, char *argv)
> {
> DEFINE_STRBUF(buf, 2);
> snappendf(buf, "kinda long string...");
> printf("buf->pos=%d\n", buf->pos);
> snappendf(buf, "will this overflow buf?");
> }
>
> $ gcc -O0 -g test.c && valgrind -q ./a.out
> buf->pos=20
> ==27408== Jump to the invalid address stated on the next line
> ==27408== at 0x6C667265766F2073: ???
> ==27408== by 0x66756220776E: ???
> ==27408== by 0x401244: snappendf (test.c:24)
> ==27408== by 0x10040003F: ???
> ==27408== by 0x1FFEFFF837: ???
> ==27408== by 0x1FFEFFF837: ???
> ==27408== by 0x3C9D8E3B01CC2FB5: ???
> ==27408== Address 0x6c667265766f2073 is not stack'd, malloc'd or (recently) free'd
> ...
>
> [0] https://gist.github.com/eddyz87/251e5f0f676a0f954d4f604c83b4922d
>
> [...]
>
I didn't bother to bullet-proofing it given it was added for
selftests, but you are right, it's trivial to guard against this:
diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index a6b972036bfa..f446432bd776 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -49,7 +49,9 @@ static inline void snappendf(struct strbuf *s, const
char *fmt, ...)
va_list args;
va_start(args, fmt);
- s->pos += vsnprintf(s->buf + s->pos, s->buf_sz - s->pos, fmt, args);
+ s->pos += vsnprintf(s->buf + s->pos,
+ s->pos < s->buf_sz ? s->buf_sz - s->pos : 0,
+ fmt, args);
va_end(args);
}
I'll do this in the next revision.
> > +#define str_has_pfx(str, pfx) \
> > + (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
>
> Nitpick: both gcc and clang optimize away strlen("foobar"),
> __builtin_constant_p check is not necessary.
it's a copy/paste from libbpf, but good to know it's actually
optimized! I can simplify it in the selftest, no problem.
>
> [...]
