[PATCH] D59005: Fix memory leak in CreateSigAltStack

[Pietro Fezzardi via Phabricator via llvm-commits]

fez marked an inline comment as done.
fez added inline comments.


================
Comment at: llvm/lib/Support/Unix/Signals.inc:255
   AltStack.ss_sp = static_cast<char *>(safe_malloc(AltStackSize));
-  NewAltStackPointer = AltStack.ss_sp; // Save to avoid reporting a leak.
   AltStack.ss_size = AltStackSize;
----------------
jfb wrote:
> fez wrote:
> > The leak shows up here, whenever the execution reaches this point two times in a single execution. The first time is fine, but the second time the first alternate signal stack leaks, because `free` is not called before reassigning `NewAltStackPointer`.
> IIUC you need to trigger two signals? How does this happen? What are the signals that trigger the leak? Do they happen concurrently?
> 
> Maybe the answer is to hold on to the alt stacks, and free them only when there's no pending signal (or not free them, ever). That seems error-prone, and the point of my refactoring was to reduce the number of existing errors in the signal handler code (which was pretty high before).
> 
> Before we do something I think we need to understand exactly what triggers this.
> IIUC you need to trigger two signals? How does this happen? What are the signals that trigger the leak? Do they happen concurrently?

No, the leak is not triggered by incoming signals, but by the fact that `CreateSigAltStack` is called multiple times. But because of the check `if (NumRegisteredSignals.load() != 0)` this can never actually happen in a multi threaded program (and neither in a single threaded one), unless for some reasons the signal handlers are initially registered (causing `NumRegisteredSignals` to be `!= 0`), then unregistered (causing `NumRegisteredSignals` to drop to 0), and then re-registered again (because after the de-registration, the check on `NumRegisteredSignals.load != 0` fails a second time).

I realize this is an extremely unlikely scenario.

At this point the only immediate advantage I see is just cleaning up Valgrind's report from this subtle false positive, which I still think is a nice thing.

I think there are three options:
- drop the patch, which leaves Valgrind complaining and effectively leaks in case of register/unregister/re-register;
- accept it like it is now, which breaks multi-threaded programs if a thread frees another's alt stack like you pointed out;
- consider the implications of my proposal above about `thread_local`, which should both fix Valgrind's false positive memory leak report, leave the multi-threaded scenario safe, and also support the register/unregister/re-register scenario (both for single- and multi-threaded).

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59005/new/

https://reviews.llvm.org/D59005