Re: getdelim: Work around buggy implementation on macOS 10.13

[Jeffrey Walton]

On Sun, Oct 23, 2022 at 11:57 PM Simon Josefsson via Gnulib discussion
list <bug-gnulib@gnu.org> wrote:
>
> Bruno Haible <bruno@clisp.org> writes:
>
> > While testing a GNU sed snapshot on macOS 10.13, I see this test failure:
> ...
> > ==85029== Invalid read of size 16
> ...
> > An out-of-bounds read. Oh oh. When I reconfigure and recompile with the
> > environment variable
> >   gl_cv_func_working_getdelim=no
> > this test succeeds. Assuming that the GNU sed code is correct (it's not a
> > particularly complex code), this proves that the out-of-bounds read comes
> > from the macOS getdelim() function.
> ...
> > +      [case "$host_os" in
> > +         darwin*)
> > +           dnl On macOS 10.13, valgrind detected an out-of-bounds read 
> > during
> > +           dnl the GNU sed test suite:
> > +           dnl   Invalid read of size 16
> > +           dnl      at 0x100EE6A05: _platform_memchr$VARIANT$Base (in 
> > /usr/lib/system/libsystem_platform.dylib)
> > +           dnl      by 0x100B7B0BD: getdelim (in 
> > /usr/lib/system/libsystem_c.dylib)
> > +           dnl      by 0x10000B0BE: ck_getdelim (utils.c:254)
> > +           gl_cv_func_working_getdelim=no ;;
>
> I don't care strongly about this issue, but this brings up a design
> perspective for gnulib wrt valgrind: we have several valgrind
> suppression files (see lib/*.valgrind) to silence valgrind complaints
> already.  Your solution here choses a different path.
>
> It can be difficult to assess wether a valgrind complaint is a false
> positive or not, especially for system functions (and we've had valgrind
> complaints for libc issues that looked problematic in theory but not in
> practice).  Unless we can trigger a real bug in the code and test for
> that, we have a choice how to handle valgrind complaints: 1) add a
> valgrind suppressions file for the complaint, or 2) unconditionally
> bring in a gnulib replacement code without testing for that behaviour.
>
> I prefer 1) since 2) will over time leads to us bringing in the entire
> gnulib replacement code on all systems, which is really bloated and
> leads to other problems.
>
> What do you think about adding a valgrind suppressions file for the
> output you found instead?  And possibly improve documentation about how
> gnulib intends these suppression files to be used by people who run the
> test-suite under valgrind, if anything is missing in that area today.

The Valgrind guide says to build at -O0 or -O1. At -O2 or above
memcheck is not accurate. From [1]:

    Compile your program with -g to include debugging
    information so that Memchecks error messages include exact
    line numbers. Using -O0 is also a good idea, if you can
    tolerate the slowdown. With -O1 line numbers in error
    messages can be inaccurate, although generally speaking
    running Memcheck on code compiled at -O1 works fairly well,
    and the speed improvement compared to running -O0 is quite
    significant. Use of -O2 and above is not recommended as
    Memcheck occasionally reports uninitialised-value errors
    which dont really exist.

Before you attempt to classify as a false positive (or not), you
should build at -O1 or possibly -O0.

[1] https://valgrind.org/docs/manual/quick-start.html

Jeff