Package: src:valkey
Version: 8.1.4+dfsg1-2
Severity: normal
Tags: patch
X-Debbugs-Cc: [email protected]
Hi,
0003-Use-get_current_dir_name-over-PATHMAX.patch frees the string returned by
get_current_dir_name() with zfree(). That string comes from glibc's malloc,
while
zfree() is Valkey's internal deallocator, and the two are not interchangeable.
The frees were added in Salsa !7 ("free allocated memory", commit 5d58f2a) and
first
shipped in 8.0.2+dfsg1-1; 8.0.1+dfsg1-1 and earlier are unaffected.
This is the same allocator mismatch as Debian #972683 and redis/redis#7927 in
2020.
#972683 was fixed in redis 5:6.0.8-2 by changing zfree(cwdp) to libc free(cwdp).
The current state in src:redis varies by suite: trixie/sid omit the frees
entirely,
bullseye uses free(), and bookworm's 5:7.0.15-1~deb12u7 still has zfree(cwdp)
in both
places, so that one has the same defect described below.
zfree() does not simply wrap free(). It calls zmalloc_size() on the pointer,
then
update_zmalloc_stat_free() and, in jemalloc builds, je_sdallocx(). None of that
is
meaningful for an allocation glibc made. The accounting call is unconditional.
For the Debian binaries the effect is accounting corruption rather than a
crash. The
package builds with USE_SYSTEM_JEMALLOC=yes, so libjemalloc.so.2 interposes
malloc and
the pointer at least belongs to the right heap. But update_zmalloc_stat_free()
still
subtracts memory that was never added, and used_memory_thread[] is size_t, so it
underflows. Building the 8.1.4+dfsg1-2 source with its full patch series and
the same
allocator flags debian/rules passes, then repeatedly failing a synchronous SAVE:
failed SAVEs used_memory
20000 376392
40000 18446744073709288008
200000 18446744073704168008
> CONFIG SET maxmemory 100mb
> SET foo bar
(error) OOM command not allowed when used memory > 'maxmemory'.
To be precise about the consequence: once the counter wraps, commands subjec
maxmemory enforcement are rejected until the server is restarted. It is not
every
write command. Scheduled BGSAVE runs in a forked child, so accounting damage
dies with the child; accumulating the wrap in the parent needs repeated
main-process
saves.
In a rebuild using upstream's bundled jemalloc instead, the same code segfau
zmalloc_size() is je_malloc_usable_size() there; handed a glibc pointer it w
rtree, finds no entry, and dereferences it:
valkey-server(je_malloc_usable_size+0x64)
valkey-server(valkey_free+0x28)
valkey-server(rdbSave+0x1cc)
valkey-server(saveCommand+0xa0)
Same top-of-stack pattern as redis/redis#7927. 30/30 on arm64 across both er
paths, and both paths on amd64. This applies to copies of patch 0003 built
without
patch 0004, which is how the patch stack tends to get reused downstream; it
property of the Debian binary.
In such a build the main-process callers of rdbSave() are SAVE, the final sn
prepareForShutdown(), FLUSHALL when save points are configured, and DEBUG RE
the server dies outright on those. FLUSHDB does not use this path.
Reproducing it: the upstream test suite already covers the rename(2) case.
tests/unit/shutdown.tcl makes dump.rdb a directory to force it. Unpack
8.1.4+dfsg1-2, drop 0004 from debian/patches/series, build with the default
allocator, and run
./runtest --single unit/shutdown
Three tests fail and the server leaves a crash report; with the patch below the
unit
passes. The fopen() path is a separate trigger, reproducible by making the d
directory unwritable for the valkey user:
Failed opening the temp RDB file temp-16585.rdb (in server root dir /tmp/r
saving: Permission denied
A minimal fix is zlibc_free(). Valkey defines it in zmalloc.c ahead of the
"#define free(ptr) je_free(ptr)" override specifically so callers can releas
allocations. A direct free(cwdp) in rdb.c does not compile, because server.h
marks
free() __attribute__((deprecated)) and Valkey builds with
-Werror=deprecated-declarations. zlibc_free() is declared in zmalloc.h in every
version from 8.0.1 through 9.1.1, and it leaves used_memory untouched, which
required behaviour here since the allocation was never counted on the way in.
--- a/debian/patches/0003-Use-get_current_dir_name-over-PATHMAX.patch
+++ b/debian/patches/0003-Use-get_current_dir_name-over-PATHMAX.patch
@@ -29,7 +29,7 @@
"Failed opening the temp RDB file %s (in server root dir
"for saving: %s",
filename, cwdp ? cwdp : "unknown", str_err);
-+ zfree(cwdp);
++ zlibc_free(cwdp);
errno = saved_errno;
return C_ERR;
}
@@ -51,7 +51,7 @@
"Error moving temp DB file %s on the final "
"destination %s (in server root dir %s): %s",
tmpfile, filename, cwdp ? cwdp : "unknown", str_err);
-+ zfree(cwdp);
++ zlibc_free(cwdp);
unlink(tmpfile);
stopSaving(0);
return C_ERR;
The same change applies to the update-to-9.1 branch behind !10, which still
carries
both calls.
For context, a Valkey maintainer noted the malloc/zfree mismatch in
valkey-io/valkey#1882 and said the project might not want the patch unmodifi
discussion did not identify the bundled-jemalloc crash, and the issue remain
valkey-io/valkey-release-automation, which builds the official Valkey binaries,
also
carries the two zfree(cwdp) calls under packaging/{7.2,8.0,8.1,9.0,9.1} on i
branch. I found no related issue or pull request there, so a fix here is worth
passing upstream.
Ubuntu package lines derived from 8.0.2 and later carry the calls; noble's 7
does not.
Regards,
Feng Ruohang
