[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:239
 static stack_t OldAltStack;
 static void* NewAltStackPointer;
 
----------------
jfb wrote:
> This comment leads me to believe that the leak is quite intentional. What happens if you free the alt stack while it's being used by another signal handler?
Good point.

For this to happen we need to assume we're are in a multi-threaded scenario, with more than one thread trying to register signal handlers and alt stacks for them.

If we assume such a scenario, then at the moment there actually is a memory leak:
- thread 0 calls `CreateSigAltStack`, and reserves a new alt stack, storing a pointer to it in the `static void *NewAltStackPointer`.
- later (never at the same time because of the mutex) thread 1 calls `CreateSigAltStack`, and reserves another new separate alt stack, storing a pointer to it again in `NewAltStackPointer`, which is `static`, hence shared among all threads. This shared `static` global gets overwritten by thread 1, effectively leaking the alt stack of thread 0.

The easiest fix I can think of, is to change `OldAltStack` and `NewAltStackPointer` to be `thread_local static` instead of just `static`. In this way we avoid both the leak (because thread 1 does not overwrite the `NewAltStackPointer` version of thread 0) and the race condition caused by one thread to free the alt stack of another thread (because each thread only allocates and frees its own `thread_local` alt stack, pointed-to by the `thread_local` version of `NewAltStackPointer`).

With this change, we could even think of moving the lock in `RegisterHandlers` right before
```
  for (auto S : IntSigs)
    registerHandler(S);
```
This should be fine, because with `CreateSigAltStack` only manipulating local and `thread_local` variables there should be no race condition anymore in `CreateSigAltStack`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59005