vdef: introduce v_counted_by_(field)

[asadsa92]

A new attribute can be used for flexible arrays to make the compiler smarter:

• https://clang.llvm.org/docs/AttributeReference.html#counted-by
• https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628459.html

This would improve the array bound sanitizer.

Note: this has not been tested locally, but this should be safe until a
compiler supports it. Once supported, we would detect and make the adjustments
as needed.

[nigoroll]

I am generally 👍🏽 , this could potentially increase our code quality. BUT

this has not been tested locally

locally installing gcc-trunk and testing the changes should not be asking too much, rather than putting the burdon on a future incarnation of some other maintainer.

[asadsa92]

this has not been tested locally

locally installing gcc-trunk and testing the changes should not be asking too much, rather than putting the burdon on a future incarnation of some other maintainer.

I agree with the reasoning to test it before merge to not uncover work later on, I was partly thinking this could be easily tested in CCI. The attribute is maybe too new. Anyways, thanks, I will look into it.

[gquintard]

@asadsa92 , do you need help with this?

[asadsa92]

@gquintard No, but thanks for the reminder.
I have added this to my backlog, I will come to it soon.

[nigoroll]

I wanted to get ahead with this feature for new code, so I built gcc trunk as of today:

./configure --prefix=/usr/local/gcc-trunk --disable-multilib --enable-languages=c
make -j10 all-gcc
make -j10 all-target-libgcc
sudo make install-gcc
sudo make install-target-libgcc

result:

$ /usr/local/gcc-trunk/bin/gcc -v
Using built-in specs.
COLLECT_GCC=/usr/local/gcc-trunk/bin/gcc
COLLECT_LTO_WRAPPER=/usr/local/gcc-trunk/libexec/gcc/x86_64-pc-linux-gnu/15.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --prefix=/usr/local/gcc-trunk --disable-multilib --enable-languages=c
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 15.0.0 20241203 (experimental) (GCC) 

issues seen:

In file included from ./cache/cache.h:46,
                 from ./cache/cache_varnishd.h:34,
                 from http2/cache_http2_deliver.c:34:
./http2/cache_http2.h:132:27: error: ‘size’ undeclared here (not in a function)
  132 |             v_counted_by_(size - PRNDUP(sizeof(struct stv_buffer)));
      |                           ^~~~

The details on the clang implementation sound like expressions were not supported for the counted_by() attribute.

Removing this offending use of v_counted_by_(), make check would succeed.

I then wanted to know if the new attribute had any effect, so I also built and installed the sanitizer:

make -j10 all-target-libsanitizer
sudo make install-target-libsanitizer

with this in place, varnishd could be compiled using ubsan

CC=/usr/local/gcc-trunk/bin/gcc CFLAGS='-g -O2 -fsanitize=bounds' ./configure --prefix=/tmp

Luckily, make check still succeeded.

[asadsa92]

@nigoroll Thanks for stepping in and giving this a test. I do offer an apology as I should have done this myself.
Good to know it worked except for the one line (feel free to remove the offending line).
Did you try to artificially make an out of bound access to see it working? If it works as expected, we should consider merging after fixing the one line.

[nigoroll]

@asadsa92 yes, I am going to finish this, but first I need to get #4240 out of the way

[nigoroll]

So now I ran an actual test with

diff --git a/lib/libvarnish/vte.c b/lib/libvarnish/vte.c
index 0ceb7d02f..6c1ee1ca7 100644
--- a/lib/libvarnish/vte.c
+++ b/lib/libvarnish/vte.c
@@ -380,6 +380,8 @@ main(int argc, char **argv)
        assert(vte->f_maxsz[0] == 4);
        assert(vte->f_maxsz[1] == 3);
        assert(vte->f_maxsz[2] == 8);
+       if (vte->f_maxsz[100])
+               WRONG("should never get here");
 
        if (strcmp(VSB_data(vsb), test_fmt)) {
                fprintf(stderr,

and learned something important: The sanitizer does not abort, it just outputs a diagnostic:

./vte_test.log:vte.c:383:18: runtime error: index 100 out of bounds for type 'int [*]'

This way, I found another glitch:

diff --git a/lib/libvarnish/vte.c b/lib/libvarnish/vte.c
index 65255e647..0ceb7d02f 100644
--- a/lib/libvarnish/vte.c
+++ b/lib/libvarnish/vte.c
@@ -63,7 +63,7 @@ struct vte {
        int             f_cnt;          /* actual number of fields */
        int             f_maxcnt;       /* maximum number of fields */
        int             f_maxsz[]
-           v_counted_by_(f_cnt);       /* maximum size per field */
+           v_counted_by_(f_maxcnt);    /* maximum size per field */
 };
 
 struct vte *

but after the fix, this remains the only place for make check:

$ find . -name \*.log| xargs grep "runtime error:"
./lib/libvarnish/vte_test.log:vte.c:383:18: runtime error: index 100 out of bounds for type 'int [*]'

[nigoroll]

I used the wrong push order, for this reason the PR is not shown as merged, but it is. The merge commit is 6f374e3

[dridi]

and learned something important: The sanitizer does not abort, it just outputs a diagnostic:

Did you export ???SAN_OPTIONS=abort_on_error=1 to the environment? Not sure which sanitizer runs this check.