From 7c3a67dda23904895a582f2f56b909b41e310036 Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Thu, 21 Jul 2016 14:37:22 +0200 Subject: [PATCH] Update valgrind-3.11.0-wrapmalloc.patch --- valgrind-3.11.0-wrapmalloc.patch | 99 ++++++++++++++++++++++++++++++++ valgrind.spec | 1 + 2 files changed, 100 insertions(+) diff --git a/valgrind-3.11.0-wrapmalloc.patch b/valgrind-3.11.0-wrapmalloc.patch index 064163f..0de544e 100644 --- a/valgrind-3.11.0-wrapmalloc.patch +++ b/valgrind-3.11.0-wrapmalloc.patch @@ -1174,3 +1174,102 @@ index 21d186b..aa879d6 100644 #endif // __PUB_TOOL_REDIR_H /*--------------------------------------------------------------------*/ +commit a80c98bab0835b51a2193aec19ce55ad607b7ec0 +Author: philippe +Date: Sat Jul 2 18:46:23 2016 +0000 + + Fix leak in m_redir.c + See below discussion for more details. + + On Sat, 2016-07-02 at 14:20 +0200, Philippe Waroquiers wrote: + > I am testing a patch (provided by Julian) that solves a false positive + > memcheck found at my work. + > + > Testing this, I decided to run valgrind under valgrind (not done since + > a long time). + > + > This shows a leak in many tests, the stack trace being such as: + > ==26246== 336 bytes in 21 blocks are definitely lost in loss record 72 of 141 + > ==26246== at 0x2801C01D: vgPlain_arena_malloc (m_mallocfree.c:1855) + > ==26246== by 0x2801D616: vgPlain_arena_strdup (m_mallocfree.c:2528) + > ==26246== by 0x2801D616: vgPlain_strdup (m_mallocfree.c:2600) + > ==26246== by 0x2801F5AD: vgPlain_redir_notify_new_DebugInfo (m_redir.c:619) + > ==26246== by 0x2803B650: di_notify_ACHIEVE_ACCEPT_STATE (debuginfo.c:771) + > ==26246== by 0x2803B650: vgPlain_di_notify_mmap (debuginfo.c:1067) + > ==26246== by 0x2806589C: vgModuleLocal_generic_PRE_sys_mmap (syswrap-generic.c:2368) + > ==26246== by 0x2809932A: vgSysWrap_amd64_linux_sys_mmap_before (syswrap-amd64-linux.c:637) + > ==26246== by 0x28061E11: vgPlain_client_syscall (syswrap-main.c:1906) + > ==26246== by 0x2805E9D2: handle_syscall (scheduler.c:1118) + > ==26246== by 0x280604A6: vgPlain_scheduler (scheduler.c:1435) + > ==26246== by 0x2806FF87: thread_wrapper (syswrap-linux.c:103) + > ==26246== by 0x2806FF87: run_a_thread_NORETURN (syswrap-linux.c:156) + > + > + > The strdup call in m_redir.c:619 was introduced by r15726. + > + > However, I am not sure this is a bug that is introduced by this change, + > or if it just reveals a leak that was already there. + > The "very original" replacement logic did not do memory allocation for + > the replacement: see m_redir.c in valgrind 3.10.1 : it was just copying + > some chars from VG_(clo_soname_synonyms) to demangled_sopatt + + Yes, it should do exactly the same as the other code paths. If + replaced_sopatt != NULL then it is an allocated string that has been + assigned to demangled_sopatt. I had assumed that would take care of the + life-time issues of the allocated string. But now that I read the code + it is indeed not so clear. + + > Then in 3.11, the fixed size demangled_sopatt was changed to be + > a dynamically allocated buffer. + > The revision log 14664 that introduced this explains that the ownership of + > returned buffer is not easy. It tells at the end: + > "So the rule of thunb here is: if in doubt strdup the string." + > + > but now we have to see when to free what, it seems ??? + > + > Any thoughts ? + + So if replaced_sopatt != NULL, then demangled_sopatt contains the + allocated string, and it is then immediately copied and assigned to + spec->from_sopatt. After that it is used under check_ppcTOCs. But there + it will first be reassigned a new value through maybe_Z_demangle + (overwriting any existing string being pointed to). So for this + particular leak it seem fine to free it right after the spec[List] has + been initialized (line 642). + + Cheers, + + Mark + + + + git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15898 a5019735-40e9-0310-863c-91ae7b9d1cf9 + +diff --git a/coregrind/m_redir.c b/coregrind/m_redir.c +index 62cb45a..c9e8726 100644 +--- a/coregrind/m_redir.c ++++ b/coregrind/m_redir.c +@@ -616,7 +616,7 @@ void VG_(redir_notify_new_DebugInfo)( const DebugInfo* newdi ) + if (replaced_sopatt == NULL + && VG_(strcmp) ( demangled_sopatt, SO_SYN_MALLOC_NAME ) == 0) + { +- replaced_sopatt = VG_(strdup)("m_redir.rnnD.1", "*"); ++ replaced_sopatt = dinfo_strdup("m_redir.rnnD.1", "*"); + demangled_sopatt = replaced_sopatt; + isGlobal = True; + } +@@ -640,6 +640,14 @@ void VG_(redir_notify_new_DebugInfo)( const DebugInfo* newdi ) + spec->mark = False; /* not significant */ + spec->done = False; /* not significant */ + specList = spec; ++ /* The demangler is the owner of the demangled_sopatt memory, ++ unless it was replaced. In this case, we have to free the ++ replace_sopatt(==demangled_sopatt). We can free it, ++ because it was dinfo_strup-ed into spec->from_sopatt. */ ++ if (replaced_sopatt != NULL) { ++ vg_assert(demangled_sopatt == replaced_sopatt); ++ dinfo_free(replaced_sopatt); ++ } + } + free_symname_array(names_init, &twoslots[0]); + } diff --git a/valgrind.spec b/valgrind.spec index 6e37bdb..2e0c441 100644 --- a/valgrind.spec +++ b/valgrind.spec @@ -522,6 +522,7 @@ echo ===============END TESTING=============== - Mandatory Perl build-requires added - Add valgrind-3.11.0-shr.patch - Add valgrind-3.11.0-pcmpxstrx-0x70-0x19.patch +- Update valgrind-3.11.0-wrapmalloc.patch * Tue Jun 21 2016 Mark Wielaard - 3.11.0-23 - Update valgrind-3.11.0-ppoll-mask.patch (#1344082)