[PATCH] D59005: Fix memory leak in CreateSigAltStack

Tue Mar 5 16:01:26 PST 2019

fez created this revision.
Herald added subscribers: llvm-commits, jdoerfert, hiraditya.
Herald added a reviewer: jfb.
Herald added a project: LLVM.

In `lib/Support/Unix/Signals.inc` there is an occasional memory leak, whenever
the function `CreateSigAltStack` is called multiple times in certain conditions.

To reproduce the bug, just run

  valgrind --leak-check=full opt -o /dev/null /dev/null

OS: Ubuntu 18.04
Valgrind version: 3.13.0
llvm version: 7.0.0 (opt-7 system installation in ubuntu, but for what I saw in the sources it affects llvm 8.0.0 as well)
You should be on a Unix system with `sigaltstack` available.

This is the leak report I get from Valgrind

  $ valgrind --leak-check=full opt-7 -o /dev/null /dev/null
  ==27771== Memcheck, a memory error detector
  ==27771== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
  ==27771== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
  ==27771== Command: opt-7 -o /dev/null /dev/null
  ==27771==
  ==27771==
  ==27771== HEAP SUMMARY:
  ==27771==     in use at exit: 67,640 bytes in 3 blocks
  ==27771==   total heap usage: 3,488 allocs, 3,485 frees, 1,069,258 bytes
  allocated
  ==27771==
  ==27771== 67,584 bytes in 1 blocks are definitely lost in loss record 3 of 3
  ==27771==    at 0x4C2FB0F: malloc (in
  /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==27771==    by 0x59622D3: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-7.so.1)
  ==27771==    by 0x5909CED: llvm::EnablePrettyStackTrace() (in
  /usr/lib/x86_64-linux-gnu/libLLVM-7.so.1)
  ==27771==    by 0x58F2C21: llvm::InitLLVM::InitLLVM(int&, char const**&) (in
  /usr/lib/x86_64-linux-gnu/libLLVM-7.so.1)
  ==27771==    by 0x1B43F7: main (in /usr/lib/llvm-7/bin/opt)
  ==27771==
  ==27771== LEAK SUMMARY:
  ==27771==    definitely lost: 67,584 bytes in 1 blocks
  ==27771==    indirectly lost: 0 bytes in 0 blocks
  ==27771==      possibly lost: 0 bytes in 0 blocks
  ==27771==    still reachable: 56 bytes in 2 blocks
  ==27771==         suppressed: 0 bytes in 0 blocks
  ==27771== Reachable blocks (those to which a pointer was found) are not shown.
  ==27771== To see them, rerun with: --leak-check=full --show-leak-kinds=all
  ==27771==
  ==27771== For counts of detected and suppressed errors, rerun with: -v
  ==27771== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I actually tracked it down in a version compiled from source with all the debug
symbols. The leak report is very similar, except that I get the exact info on
the leaking allocation:

  ==20704== HEAP SUMMARY:
  ==20704==     in use at exit: 1,020,675 bytes in 35 blocks
  ==20704==   total heap usage: 292,536 allocs, 292,501 frees, 49,368,073 bytes
  allocated
  ==20704==
  ==20704== 67,584 bytes in 1 blocks are definitely lost in loss record 14 of 27
  ==20704==    at 0x4C2FB0F: malloc (in
  /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==20704==    by 0x893D63C: safe_malloc (MemAlloc.h:27)
  ==20704==    by 0x893D63C: CreateSigAltStack (Signals.inc:254)
  ==20704==    by 0x893D63C: RegisterHandlers() (Signals.inc:277)
  ==20704==    by 0x893DF9C: llvm::sys::AddSignalHandler(void (*)(void*), void*)
  (Signals.inc:386)
  ==20704==    by 0x88E538D: RegisterCrashPrinter (PrettyStackTrace.cpp:180)
  ==20704==    by 0x88E538D: llvm::EnablePrettyStackTrace()
  (PrettyStackTrace.cpp:188)
  ==20704==    by 0x88CFFED: PrettyStackTraceProgram (PrettyStackTrace.h:77)
  ==20704==    by 0x88CFFED: llvm::InitLLVM::InitLLVM(int&, char const**&)
  (InitLLVM.cpp:25)
  ==20704==    by 0x41DF07: InitLLVM (InitLLVM.h:35)
  ==20704==    by 0x41DF07: main (opt.cpp:426)

The leak shows up whenever `CreateSigAltStack` is called multiple times, in some
specific conditions.

The first time `CreateSigAltStack` is called, a new alternate signal stack
isallocated with

  AltStack.ss_sp = static_cast<char *>(safe_malloc(AltStackSize));

Then the value of `AltStack.ss_sp` is saved into NewAltStackPointer. According
to the comments, this is a workaround to stop valgrind or asan complaining about
a leak, but it's not enough. Here's why. Whenever `CreateSigAltStack` is called
a second time and it reaches past the first if, a new larger alternate stack is
allocated again with

  AltStack.ss_sp = static_cast<char *>(safe_malloc(AltStackSize));

But at this point, the `NewAltStackPointer` is overwritten, and the old
alternate signal stack is never free'd, and we don't hold a pointer to it
anymore, causing the leak.

With this fix the leak should go away. The valgrind leak report report goes away
as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59005

Files:
  llvm/lib/Support/Unix/Signals.inc


Index: llvm/lib/Support/Unix/Signals.inc
===================================================================

--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -252,10 +252,13 @@
 
   stack_t AltStack = {};
   AltStack.ss_sp = static_cast<char *>(safe_malloc(AltStackSize));
-  NewAltStackPointer = AltStack.ss_sp; // Save to avoid reporting a leak.
   AltStack.ss_size = AltStackSize;
-  if (sigaltstack(&AltStack, &OldAltStack) != 0)
+  if (sigaltstack(&AltStack, &OldAltStack) != 0) {
     free(AltStack.ss_sp);
+  } else {
+    free(NewAltStackPointer);
+    NewAltStackPointer = AltStack.ss_sp; // Save to avoid reporting a leak.
+  }
 }
 #else
 static void CreateSigAltStack() {}


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59005.189416.patch
Type: text/x-patch
Size: 714 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190306/185d206f/attachment.bin>
