Calling realloc with a size of zero is so confusing that C11 deprecated it http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_400 Given the above it is probably always a mistake when realloc is called with size == 0 so it would be good to warn about that. Currently vg_replace_malloc treats size <= 0 as free and returns NULL.
How can we tell C11 code from pre-C11 code? I have seen memory management code that uses realloc like this, but it is probably a rather rare use case.
We cannot tell pre-C11 code from C11 code, but realloc with size zero has always been problematic because the the behavior has been implementation defined and different implementations decided to implement it differently (some freed the original pointer, some returned an invalid pointer, some returned NULL, some set errno, etc.) So IMHO calling realloc with size zero should always be flagged.
Another interesting corner case is malloc (0), this is also implementation defined. It might either return a NULL pointer or an unique pointer (that needs to be freed). Currently valgrind doesn't warn about this, but does report such blocks as lost when not freed: definitely lost: 0 bytes in 1 blocks If we warn about realloc with size zero we might also want to warn about malloc with size zero. Both calls are suspect and might not do what the developer expects.
I've been thinking about this. I don't think that there is a quick fix. Some of this may also apply to alligned alloc functions that I've also been thinking about. What needs doing? First a new option --realloc-size-zero-free=yes|no warn when realloc is used with a size of zeroy [no] I think that the REALLOC macro in vg_replace_malloc.c needs to just be a wrapper around tool.realloc and not decide what to do with different values of pointer and size. That does mean that all the tool realloc replacements will need updating.
I'm getting close to having a patch for this. I need to check that all tools work OK and also check that suppressions work. I'll try on-by default to begin with. This will print both the realloc==free context and the allocation context. In the case of realloc(invalid, 0) two warnings will be printed.
The precise wording of the C17 standard is [7.22.3.5 The realloc function para 3] If size is zero and memory for the new object is not allocated, it is implementation-defined whether the old object is deallocated. If the old object is not deallocated, its value shall be unchanged. I take that to mean two things are possible. 1. A non-zero sized non-dereferenceable block is rerturned and the old memory is freed 2. No memory is allocated and the old memory may or may not be freed. In any case if the memory is not freed then the pointer isn't changed.
The wording is getting a bit tougher in C2X If ptr is a null pointer, the realloc function behaves like the malloc function for the specified size. Otherwise, if ptr does not match a pointer earlier returned by a memory management function, or if the space has been deallocated by a call to the free or realloc function, or if the size is zero, the behavior is undefined. If memory for the new object is not allocated, the old object is not deallocated and its value is unchanged Implementation defined -> undefined.
I've done as much testing as I can (with a slight exception for musl where I just looked at the source). Universally, if ptr is 0 then the call behaves like malloc. My initial feeling is that we shouldn't warn in that case. Where things diverge is when the size is 0 (which is what this is all about). Linux glibc calls free and returns NULL. Even here that is not necessarily the case as there is a macro REALLOC_ZERO_BYTES_FREES that controls it (by default it is activated). Illumos also frees and returns NULL (with no macro). All other platforms call free and allocate "something". I don't have a 100% clear picture of the size of that something. On FreeBSD (which uses jemalloc) the size is bumped up to 1. Musl leaves it at 0. I don't have access to the Solaris source, but 11.3 doesn't return NULL at least. Need to double check that. I'd like this behaviour to try to be as much like the underlying system as possible. So I'll add a second option --realloc-zero-bytes-frees=[yes|no]. The default will depend on the platform. When yes the behaviour will be like glibc, when no the behaviour will be like jemalloc. I don't see any practical way to determine this at runtime. That should allow someone who is using musl libc with a glibc built Valgrind to switch.
Created attachment 155503 [details] warn about realloc zero OK here it is. A big patch. Now warns about realloc 0. Two new options, one core to switch between glibc behaviour and other behaviour. One mc option to turn off these warnings. A new suppression tag. I move all the 0-size and null-ptr checks from the core wrapper to the tools. Only tested on FreeBSD for the moment.
memcheck/tests/Makefile.am:1: error: realloc_size_zero_mismatch.stdout.exp is missing in EXTRA_DIST memcheck/tests/Makefile.am:1: error: realloc_size_zero_mismatch.vgtest is missing in EXTRA_DIST memcheck/tests/Makefile.am:1: error: realoc_size_zero_mismatch.vgtest is in EXTRA_DIST but doesn't exist will fix
Created attachment 155510 [details] warn about realloc zero fix EXTRA_DIST
(In reply to Paul Floyd from comment #11) > Created attachment 155510 [details] > warn about realloc zero > > fix EXTRA_DIST This looks like the patch from bug #452802 (which has already been applied).
> This looks like the patch from bug #452802 (which has already been applied). Bah! The previous version was (almost) OK.
Created attachment 155567 [details] Try again With the right patch file this time hopefully.
...checking header files and include directives *** File coregrind/m_replacemalloc/vg_replace_malloc.c must not include pub_tool_options.h Only core headers are allowed in non-tool source files. I assume this is for checking info.clo_realloc_zero_bytes_frees This seems to work, but I don't fully understand how. This is part of the vgpreload library, how does that have access to the tools variables?
diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index 401a7ba8e..13d3b6bf2 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -6468,8 +6468,9 @@ PRE(sys___sysctlbyname) SET_STATUS_Success(0); } - // @todo PJF kern.proc.pathname - // how is that done? jusr a pid or -1 in the string? + // kern.proc.pathname doesn't seem to be handled + // makes sense as the pid is variable and using + // a MIB is easier than generating a string // read number of ints specified in ARG2 from mem pointed to by ARG1 PRE_MEM_READ("__sysctlbyname(name)", (Addr)ARG1, ARG2 * sizeof(int)); This part seems unrelated
(In reply to Mark Wielaard from comment #16) > This part seems unrelated Yes, i must have forgotten that I'd switched branches. I just pushed this bit to master and will update the patch.
(In reply to Mark Wielaard from comment #15) > ...checking header files and include directives > *** File coregrind/m_replacemalloc/vg_replace_malloc.c must not include > pub_tool_options.h > > Only core headers are allowed in non-tool source files. > > I assume this is for checking info.clo_realloc_zero_bytes_frees > This seems to work, but I don't fully understand how. > This is part of the vgpreload library, how does that have access to the > tools variables? It did strike me as being a bit out of place. The global "clo" variables do get declared in both core and tool headers, but not both (you can see this with find . -name "*.h" -exec grep "VG_(clo_" {} /dev/null \; | grep extern ) Which makes me think that ./include/pub_tool_replacemalloc.h is better. Looking a bit more, it should be in replacement_malloc_process_cmd_line_option since this is an option that only applies to the tools that replace malloc (dh drd hg ms mc). I'll make the change tonight and update the patch.
Created attachment 155629 [details] Move arg to replacemalloc_core I think that this is cleaner now.
Created attachment 156450 [details] Update news
Looks good to me.
commit 50bded71b23cb11a8b6c1b6eaf6e3abcc05a06c2 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Fri Mar 10 21:55:14 2023 +0100 Bug 436413 - Warn about realloc of size zero Adds a new warning to memcheck when realloc is used with a size of 0. For a long time this has been "implementation defined" and so non-portable. With C23 it will become UB. Also adds a switch to turn off the error generation and a second switch to select between the most common "implementation" behaviours. The defaults for this second switch are baked in at build time.