> 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.
It is a maintenance burden in a way.
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
[...]
> +#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.
[...]
