Bug 476108 - vg_replace_malloc DELETE checks size
Summary: vg_replace_malloc DELETE checks size
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-26 10:33 UTC by Mark Wielaard
Modified: 2023-10-26 10:55 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2023-10-26 10:33:26 UTC
The DELETE replacement functions check the size argument, but this doesn't actually exist.
Only the DELETE_SIZED replacement functions get a size (and should check it).

On i386 (fedora gnu/linux) this causes the following failures:

memcheck/tests/cxx17_aligned_new         (stderr)
memcheck/tests/leak_cpp_interior         (stderr)
memcheck/tests/mismatches                (stderr)
memcheck/tests/mismatches_xml            (stderr)
memcheck/tests/new_aligned_delete_default (stderr)
memcheck/tests/new_nothrow               (stderr)
memcheck/tests/realloc_size_zero_mismatch (stderr)

All showing "size" being undefined:

+Conditional jump or move depends on uninitialised value(s)
+   at 0x........: ...operator delete[]... (vg_replace_malloc.c:...)

or 

+Mismatched new/delete size value: 4
+   at 0x........: ...operator delete... (vg_replace_malloc.c:...)

Oddly no other architecture seems to show issues. Maybe we just got lucky?

The following patch fixes the issues on i386 (and shows no regressions on x86_64):

diff --git a/coregrind/m_replacemalloc/vg_replace_malloc.c b/coregrind/m_replacemalloc/vg_replace_malloc.c
index e238a52f3..7859f5f32 100644
--- a/coregrind/m_replacemalloc/vg_replace_malloc.c
+++ b/coregrind/m_replacemalloc/vg_replace_malloc.c
@@ -1027,13 +1027,12 @@ extern int * __error(void) __attribute__((weak));
 
 #define DELETE(soname, fnname, vg_replacement, tag) \
  \
-    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p, SizeT size); \
-    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p, SizeT size)  \
+    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p); \
+    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p)  \
  { \
  struct AlignedAllocInfo aligned_alloc_info = { .mem=p, .alloc_kind=AllocKind##tag }; \
       \
       DO_INIT; \
-      TRIGGER_MEMCHECK_ERROR_IF_UNDEFINED((UWord)size); \
       VERIFY_ALIGNMENT(&aligned_alloc_info); \
       MALLOC_TRACE(#fnname "(%p)\n", p ); \
       if (p == NULL)  \
Comment 1 Mark Wielaard 2023-10-26 10:54:15 UTC
commit a8b6ee6b5f5efbd759c87fa987e9149800db2899
Author: Mark Wielaard <mark@klomp.org>
Date:   Thu Oct 26 12:25:44 2023 +0200

    vg_replace_malloc DELETE should not check size
    
    The DELETE replacement functions check the size argument, but this
    doesn't actually exist.  Only the DELETE_SIZED replacement functions
    get a size (and should check it).
    
    On i386 (fedora gnu/linux) this causes the following failures:
    
    memcheck/tests/cxx17_aligned_new         (stderr)
    memcheck/tests/leak_cpp_interior         (stderr)
    memcheck/tests/mismatches                (stderr)
    memcheck/tests/mismatches_xml            (stderr)
    memcheck/tests/new_aligned_delete_default (stderr)
    memcheck/tests/new_nothrow               (stderr)
    memcheck/tests/realloc_size_zero_mismatch (stderr)
    
    All showing "size" being undefined:
    
    +Conditional jump or move depends on uninitialised value(s)
    +   at 0x........: ...operator delete[]... (vg_replace_malloc.c:...)
    
    or
    
    +Mismatched new/delete size value: 4
    +   at 0x........: ...operator delete... (vg_replace_malloc.c:...)
    
    Oddly no other architecture seems to show issues. Maybe we just got lucky?
    
    This patch fixes the issues on i386 (and shows no regressions on x86_64)
    
    https://bugs.kde.org/show_bug.cgi?id=476108
Comment 2 Paul Floyd 2023-10-26 10:55:14 UTC
Yes, I probably copied DELETE from DELETE_SIZED and didn’t remove all the sized bits.

On 64bit the rags are passed in registers so less likely to be uninitialized.

The diff looks good.