Use After Free error

[koehn]

Thanks for this project; I was able to migrate off of Redis 7.4 after I inadvertently upgraded my databases.

To get it to build under Debian I had to comment out -Werror from deps/redis/Makefile as it was complaining about a potential use-after-free error. With that commented out it built and ran fine (once I figured out I needed to clone hiredis; might want to add that to the documentation).

[moticless]

Hi @koehn,
I created PR that should resolve the hiredis issue.
Regarding the use-after-free can you dig little further please to see if this is a real issue?
Thank you.

[koehn]

Here’s what I get with the new Makefiles using a clean debian:latest docker image:

# apt update && apt-get install -y build-essential cmake make git
[snip]
# git clone https://github.com/redis/librdb.git
[cd into librdb and apply your changes to the makefiles]
# make example
git submodule update --init deps/hiredis
Submodule 'deps/hiredis' (https://github.com/redis/hiredis.git) registered for path 'deps/hiredis'
Cloning into '/librdb/deps/hiredis'...
Submodule path 'deps/hiredis': checked out '869f3d0ef1513dd0258ad7190c9914df16dcc4a4'
make -C deps  all
make[1]: Entering directory '/librdb/deps'
make -C redis all
make[2]: Entering directory '/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 crc64.c > crc64.d
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c crcspeed.c -o crcspeed.o -g3 -DDEBUG=1
cc -MM -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden crcspeed.c > crcspeed.d
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c endianconv.c -o endianconv.o -g3 -DDEBUG=1
cc -MM -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden endianconv.c > endianconv.d
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c fpconv_dtoa.c -o fpconv_dtoa.o -g3 -DDEBUG=1
cc -MM -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden fpconv_dtoa.c > fpconv_dtoa.d
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c intset.c -o intset.o -g3 -DDEBUG=1
cc -MM -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden intset.c > intset.d
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c listpack.c -o listpack.o -g3 -DDEBUG=1
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
make[2]: *** [Makefile:15: listpack.o] Error 1
make[2]: Leaving directory '/librdb/deps/redis'
make[1]: *** [Makefile:2: all] Error 2
make[1]: Leaving directory '/librdb/deps'
make: *** [Makefile:26: all] Error 2

So it’s cloning hiredis, which is good. Looks like I’m still needing to uncomment -Werror, which may be another issue.

[koehn]

Here’s a Dockerfile that will reproduce the test case:

FROM debian:latest

RUN apt-get update && apt-get install -y build-essential git make

RUN git clone https://github.com/redis/librdb.git

# Uncomment the line below to get a successful build
#RUN sed 's/^\(.*\) -Werror/\1/' -i librdb/deps/redis/Makefile

RUN cd librdb && make example

Save that to a file named Dockerfile, then run docker build .

Uncomment the RUN sed line to make the build pass.

[koehn]

Looking at the offending code, it seems like the compiler is just pitching a fit.

    /* Store the offset of the element 'first', so that we can obtain its
     * address again after a reallocation. */
    unsigned long poff = first-lp;

    /* Move tail to the front of the listpack */
    memmove(first, tail, eofptr - tail + 1);
    lpSetTotalBytes(lp, bytes - (tail - first));
    uint32_t numele = lpGetNumElements(lp);
    if (numele != LP_HDR_NUMELE_UNKNOWN)
        lpSetNumElements(lp, numele-deleted);
    lp = lpShrinkToFit(lp);

    /* Store the entry. */
    *p = lp+poff;
    if ((*p)[0] == LP_EOF) *p = NULL;

    return lp;

My guess is (I haven’t done any serious C coding since the nineties) if you simply used a different variable to hold the results of lpShrinkToFit() instead of re-assigning it to lp, the compiler wouldn’t throw a fit about use after free. I’ll try to make a branch and test it out.

[koehn]

Hmm. Changing the assignment to a new variable didn’t help; the compiler is convinced that one of the preceding (inlined) functions is calling realloc, apparently in lpAssertValidEntry() (as that’s the last place it’s used).

I’m at a loss.

[moticless]

I reproduced the warnning. It doesn't look like a real problem.
Played a little with warnning flags of deps/Makefile. When I optimized it with flag -flto=auto the issue disappear like magic.
Please verify it works for you as well.

[koehn]

That works for me!

[wdoekes]

Hi guys. I ran into this right now when building sonic-net/sonic-buildimage with debian:bookworm, which uses: gcc (Debian 12.2.0-14) 12.2.0 (dpkg gcc 4:12.2.0-3)

The -flto=auto fix helps indeed.

Build default:

make
...
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -Werror -fvisibility=hidden -c crc64.c -o crc64.o -g3 -DDEBUG=1 

Building with altered flags:

make CFLAGS='-fPIC -O3 -std=c99 -Wall -Wextra -pedantic -fvisibility=hidden -flto=auto'
...
cc -fPIC -O3 -std=c99 -Wall -Wextra -pedantic -fvisibility=hidden -flto=auto -c crc64.c -o crc64.o -g3 -DDEBUG=1 

Without the -flto=auto I get the following:

listpack.c: In function 'lpDeleteRangeWithEntry':
listpack.c:937:31: warning: pointer 'lp' used after 'realloc' [-Wuse-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);
      |                ^~~~~~~~~~

rax.c: In function 'raxRemove':
rax.c:1064:28: warning: pointer 'h' may be used after 'free' [-Wuse-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);
      |             ^~~~~~~~

And with -flto=auto I get these instead:

In function 'allocEmbeddedBulk',
    inlined from 'hashListPack' at ../lib/parser.c:1156:14:
../lib/parser.c:1042:35: warning: 'valueVal' may be used uninitialized [-Wmaybe-uninitialized]
 1042 |         embeddedBulk->binfo.len = ll2string(embeddedBulk->binfo.ref, buflen, sval);
      |                                   ^
../lib/parser.c: In function 'hashListPack':
../lib/parser.c:1135:29: note: 'valueVal' was declared here
 1135 |         long long fieldVal, valueVal;
      |                             ^

In function 'allocEmbeddedBulk',
    inlined from 'hashListPack' at ../lib/parser.c:1153:14:
../lib/parser.c:1042:35: warning: 'fieldVal' may be used uninitialized [-Wmaybe-uninitialized]
 1042 |         embeddedBulk->binfo.len = ll2string(embeddedBulk->binfo.ref, buflen, sval);
      |                                   ^
../lib/parser.c: In function 'hashListPack':
../lib/parser.c:1135:19: note: 'fieldVal' was declared here
 1135 |         long long fieldVal, valueVal;
      |                   ^

In function 'allocEmbeddedBulk',
    inlined from 'elementZsetLP' at ../lib/parser.c:2087:18:
../lib/parser.c:1042:35: warning: 'item1Val' may be used uninitialized [-Wmaybe-uninitialized]
 1042 |         embeddedBulk->binfo.len = ll2string(embeddedBulk->binfo.ref, buflen, sval);
      |                                   ^
../lib/parser.c: In function 'elementZsetLP':
../lib/parser.c:2057:15: note: 'item1Val' was declared here
 2057 |     long long item1Val, item2Val;
      |               ^

Awkward. Both those cases are falsely flagged by the compiler. (When fieldVal and item1Val are indeed uninitialized, field and item1 are non-NULL, so we do not reach 1042, but 1035.

[JunhongMao]

Hi @moticless, what's your opinion? Hope this issue can be fixed ASAP. Thanks.

[moticless]

Hi @JunhongMao ,
I’m might get to this by the end of the week. Maybe you be able to assist with fixing it? Thanks.

[JunhongMao]

Yes. My pleasure.

Hi @JunhongMao , I’m might get to this by the end of the week. Maybe you be able to assist with fixing it? Thanks.