redis-cli build broken on Debian/Bookworm (librdb use-after-free)

[wdoekes]

Description

In 9685498 redis-rdb-tool is replaced by rdb-cli.

The new build of redis-cli fails on Debian/Bookworm because of this (seemingly) false positive compiler error:
redis/librdb#55

Looks like this:

Submodule 'deps/hiredis' (https://github.com/redis/hiredis.git) registered for path 'deps/hiredis'
Cloning into '/sonic/src/rdb-cli/librdb/deps/hiredis'...
Submodule path 'deps/hiredis': checked out '869f3d0ef1513dd0258ad7190c9914df16dcc4a4'
make[2]: Entering directory '/sonic/src/rdb-cli/librdb'
make -C deps  all
make[3]: Entering directory '/sonic/src/rdb-cli/k/deps'
make -C redis all
make[4]: Entering directory '/sonic/src/rdb-cli/librdb/deps/redis'
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c crc64.c -o crc64.o -g3 -DDEBUG=1
...
cc -MM -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden zipmap.c > zipmap.d
rax.c: In function 'raxRemove':
rax.c:1064:28: error: pointer 'h' may be used after 'free' [-Werror=use-after-free]
 1064 |             raxNode *new = raxRemoveChild(h,child);
      |                            ^~~~~~~~~~~~~~~~~~~~~~~
In file included from rax.c:45:
rax_malloc.h:44:18: note: call to 'free' here
   44 | #define rax_free free
rax.c:1054:13: note: in expansion of macro 'rax_free'
 1054 |             rax_free(child);
      |             ^~~~~~~~
listpack.c: In function 'lpDeleteRangeWithEntry':
listpack.c:937:31: error: pointer 'lp' used after 'realloc' [-Werror=use-after-free]
  937 |     unsigned long poff = first-lp;
      |                          ~~~~~^~~
In file included from listpack.c:45:
In function 'lpShrinkToFit',
    inlined from 'lpDeleteRangeWithEntry' at listpack.c:945:10:
listpack_malloc.h:47:28: note: call to 'realloc' here
   47 | #define lp_realloc(ptr,sz) realloc(ptr,sz)
      |                            ^~~~~~~~~~~~~~~
listpack.c:177:16: note: in expansion of macro 'lp_realloc'
  177 |         return lp_realloc(lp, size);
      |                ^~~~~~~~~~
cc1: all warnings being treated as errors

Steps to reproduce the issue:

1. make target/files/bookworm/rdb-cli

Workaround

Disable -Werror and maybe add -flto=auto per the rdb issue 55 mentioned above:

--- a/src/rdb-cli/Makefile
+++ b/src/rdb-cli/Makefile
@@ -10,5 +10,6 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
        pushd ./librdb/
        git checkout 2fdfc0c2bc914d643fe3f86e6715aeb843d8966e
        git submodule update --init --recursive 
-       make -j$(SONIC_CONFIG_MAKE_JOBS)
+       # Set WARNS=... to work around https://github.com/redis/librdb/issues/55
+       make -j$(SONIC_CONFIG_MAKE_JOBS) WARNS='-Wall -Wextra -pedantic -flto=auto'
        mv bin/rdb-cli $(DEST)/

Proper fix

Not sure.

• Wait for compiler fix?
• Wait for/implement workaround in upstream (librdb)?

Cheers,
Walter Doekes
OSSO B.V.

[saiarcot895]

Is adding just -flto=auto sufficient for fixing it? Or do -Wall -Wextra -pedantic also need to be added?

[JunhongMao]

The original Makefile is below. I guess

librdb$ vi deps/redis/Makefile
SOURCES = $(notdir $(basename $(wildcard *.c)))
OBJECTS = $(patsubst %,%.o,$(SOURCES))

OPTIMIZATION?=-O3

STD            = -std=c99
WARNS          = -Wall -Wextra -pedantic -Werror
CFLAGS         = -fPIC $(OPTIMIZATION) $(STD) $(WARNS) -fvisibility=hidden
DEBUG          = -g3 -DDEBUG=1
LIBS           =

The below will replace the original WARNS. So "-Wall -Wextra -pedantic" is also needed.

 make -j$(SONIC_CONFIG_MAKE_JOBS) WARNS='-Wall -Wextra -pedantic -flto=auto'

I have also created a new PR below to fix it. Please help to review and approve it. Thanks.
#20759

[wdoekes]

The below will replace the original WARNS. So "-Wall -Wextra -pedantic" is also needed.

Correct. They're technically not needed, but it makes for the smallest change with upstream.

As mentioned in the upstream bug report: removing -Werror is needed and adding -flto=auto makes the warning less severe. ("Use after free" sounds worse than "possibly uninitialized value.")

[wdoekes]

An alternative fix might be to prefer gcc-11 on Bookworm. There are various reports that gcc-12 is not that good.

diff --git a/sonic-slave-bookworm/Dockerfile.j2 b/sonic-slave-bookworm/Dockerfile.j2
index 145ba7d0a..58d00cfcd 100644
--- a/sonic-slave-bookworm/Dockerfile.j2
+++ b/sonic-slave-bookworm/Dockerfile.j2
@@ -25,6 +25,18 @@ COPY ["no-check-valid-until", "/etc/apt/apt.conf.d/"]
 
 {%- if CROSS_BUILD_ENVIRON != "y" %}
 COPY ["sources.list.{{ CONFIGURED_ARCH }}", "/etc/apt/sources.list"]
+# gcc-12 in bookworm has trouble with certain false positives. Prefer gcc-11
+# instead.
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1006803
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1008630
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1032130
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106776
+# https://github.com/redis/librdb/issues/55
+RUN apt-get install -y gcc-11 g++-11 && \
+    update-alternatives --install /usr/bin/cc cc /usr/bin/gcc-11 20 && \
+    update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-11 20 && \
+    update-alternatives --set cc /usr/bin/gcc-11 && \
+    update-alternatives --set c++ /usr/bin/g++-11
 {%- else %}
 COPY ["sources.list.amd64", "/etc/apt/sources.list"]
 {%- if CONFIGURED_ARCH == "armhf" %}

This works too.

If we only observe trouble with librdb, we can probably go with the former patch. But if more compile warnings turn up as false positive errors, then this could be an alternative fix.