[libunwind] Fix memory leak in handling of DW_CFA_remember_state and DW_CFA_restore_state

[jgorbe]

Summary

parseInstructions() doesn't always process the whole set of DWARF instructions for a frame. It will stop once the target PC is reached, or if malformed instructions are found. So, for example, if we have an instruction sequence like this:

<start>
...
DW_CFA_remember_state
...
DW_CFA_advance_loc past the location we're unwinding at (pcoffset in parseInstructions() main loop)
...
DW_CFA_restore_state
<end>

... the saved state will never be freed, even though the DW_CFA_remember_state opcode has a matching DW_CFA_restore_state later in the sequence.

This change adds code to free whatever is left on rememberStack after parsing the CIE and the FDE instructions.

[dblaikie]

Can you include a test case for this? (I have no idea what the test coverage/options are for libunwind, so "that's not possible" is a possible answer (though I'd be sad if that's the case))

[jgorbe]

Looking into how to add a test now. I'm not familiar at all with libunwind code, so I have no idea how to cause this leak to happen from the outside (and then the test would only work when running under asan). Any pointers would be much appreciated.

[dblaikie]

In D66904#1653092, @jgorbe wrote:

Looking into how to add a test now. I'm not familiar at all with libunwind code, so I have no idea how to cause this leak to happen from the outside (and then the test would only work when running under asan). Any pointers would be much appreciated.

It looks like libunwind is fairly undertested by my estimate - but it does have test files that are, by the looks of it, compiled and run and expected to exit 0? (the .cpp files in the libunwind/test directory)

So you might be able to test this by adding a test file that has a hardcoded assembly eh_frame (if you can use "REQUIRES" to limit it to only run on platforms where your assembly is appropriate) & if the test would only fail with sanitizers, that's OK (and you might be able to make the test only run with sanitizers with another REQUIRES entry - I'd suggest checking llvm tests to see if any of them have/support/use a filter on sanitizers - but that'd just speed up things on non-sanitizer builds, given how few tests are here that's hardly likely to be a critical problem).

[jgorbe]

Added a test case that seems to work well with the existing testing setup if I rebuild LLVM with ASan enabled. I have checked out other usages of .cfi_* directives in other LLVM assembly test files and they seem to run them through some llvm tool with -triple <something including linux>, so I have added a REQUIRES: linux line to avoid breakage.

I don't know if there's a better way of dealing with tests that need ASan here, or if I have been too strict by requiring linux. There aren't a lot of files in libunwind/test so there has been some guessing and extrapolation on my side (and a lot of help from dblaikie!). Again, any advice will be very welcome.

[dblaikie]

libunwind/test/remember_state_leak.pass.sh.s

5 ↗

(On Diff #219207)

Other tests were written as .cpp file - is there a reason this one is all in assembly, compared to the .cpp with inline asm example you've given in the comment here?

[jgorbe]

It's just that it seemed weird/hacky to use asm just to stick assembler directives in the middle of the generated code instead of actual assembly, so I generated an assembly version and cleaned it up a bit. I'm happy to go back to the original shorter C++ version if that's not a concern.

[jgorbe]

libunwind/test/remember_state_leak.pass.sh.s

5 ↗

(On Diff #219207)

See reply in commit-level comment.

[dblaikie]

In D66904#1661872, @jgorbe wrote:

It's just that it seemed weird/hacky to use asm just to stick assembler directives in the middle of the generated code instead of actual assembly, so I generated an assembly version and cleaned it up a bit. I'm happy to go back to the original shorter C++ version if that's not a concern.

Fair enough - I /think/ I'd lean towards source with inline assembly, but do feel free to wait for someone with more knowledge about this code/who can sign off in any case (with or without this change and others).

[dblaikie]

Hey @compnerd - is this something you can take a look at/feel comfortable reviewing? I know /so/ little this stuff I'd appreciate a second set of eyes.

[compnerd]

I think that using assembly for the test is fine

libunwind/test/remember_state_leak.pass.sh.s

1 ↗

(On Diff #219207)

Can you add a REQUIRES: x86 as well please? This shouldnt be executed on non-x86 platforms. I think that explicitly listing the triple is a good idea in the build rules (you could default to x86 builds which wont work with x86_64). I think that we *could* add a requirement on ASAN as well, but, that isn't as big of a concern.

39 ↗

(On Diff #219207)

Mind extracting this value out into a macro (SIZEOF_UNWIND_EXCEPTION) so others can easily identify where the value comes from?

49 ↗

(On Diff #219207)

I'm not sure I understand the need for the spill. Did you form this from -O0? I imagine that DSE would remove these three instructions.

[jgorbe]

Re-made the test asm file starting with a -O2 compilation to avoid some unnecessary code.

libunwind/test/remember_state_leak.pass.sh.s

1 ↗

(On Diff #219207)

I agree that this shouldn't run on non-x86. Thanks for catching this. That said, shouldn't it be REQUIRES: x86_64 or similar? The asm code below uses 64-bit registers.

Anyway, I just tried both REQUIRES: x86 and REQUIRES: x86_64, and when I run the tests with ninja check-unwind on my x86_64 machine the test doesn't run and it's counted within "Unsupported Tests" in the final summary instead.

What do you mean by listing the triple in the build rules? I can't find any example of it in the existing tests and my knowledge of the testing infrastructure has a lot of gaps :(

39 ↗

(On Diff #219207)

sizeof(_Unwind_Exception) seems to be 32 instead. I didn't build the original C++ file with optimizations, so there was probably a frame pointer in there too.

I have rebuilt the example with -O2 and now it's subtracting 40 from rsp, which I believe is equal to 8 bytes to align the stack to a 16-byte boundary plus 32 for the exception object. Would you prefer to have two sub instructions instead, one that adjusts the stack and another one that reserves 32 additional bytes using a SIZEOF_UNWIND_EXCEPTION macro? (and then two matching add instructions in the epilog?)

49 ↗

(On Diff #219207)

I didn't specify any optimization options, so I guess it was -O0 by default. I have rebuilt it with -O2 now, which indeed produces more concise code here. Thanks!

[compnerd]

libunwind/test/remember_state_leak.pass.sh.s

1 ↗

(On Diff #219207)

Nope, it should be x86. The backend is called x86 and supports (kinda) 16-bit, 32-bit and 64-bit code.

As to the triple, what I mean is that we should add a -target x86_64-unknown-linux-gnu to the build invocation (you should be able to add that after %build)

39 ↗

(On Diff #219207)

I think that the readability of the test is more important, so lets go with the two subs.

[jgorbe]

Sorry it took me so long to respond to this. Please take another look.

libunwind/test/remember_state_leak.pass.sh.s

1 ↗

(On Diff #219207)

Added x86 to the REQUIRES line and a target triple to the %build invocation.

39 ↗

(On Diff #219207)

Done.

[jgorbe]

Ping

[dblaikie]

@compnerd - any chance you could take another look/sign off on this?

[jgorbe]

Ping?

[jgorbe]

Could anyone please take another look at this?

[lhames]

LGTM. Thanks Jorge!

libunwind/src/DwarfParser.hpp

bool CFI_Parser<A>::parseFDEInstructions(A &addressSpace,
                                          const FDE_Info &fdeInfo,
                                          const CIE_Info &cieInfo, pint_t upToPC,
                                          int arch, PrologInfo *results) {
   // clear results
   memset(results, '\0', sizeof(PrologInfo));
   PrologInfoStackEntry *rememberStack = NULL;
 
   // parse CIE then FDE instructions
-  return parseInstructions(addressSpace, cieInfo.cieInstructions,
-                           cieInfo.cieStart + cieInfo.cieLength, cieInfo,
-                           (pint_t)(-1), rememberStack, arch, results) &&
-         parseInstructions(addressSpace, fdeInfo.fdeInstructions,
-                           fdeInfo.fdeStart + fdeInfo.fdeLength, cieInfo,
-                           upToPC - fdeInfo.pcStart, rememberStack, arch,
-                           results);
+  bool returnValue =
+      parseInstructions(addressSpace, cieInfo.cieInstructions,
+                        upToPC - fdeInfo.pcStart, rememberStack, arch, results);
+
+  // Clean up rememberStack. Even in the case where every DW_CFA_remember_state
+  // is paired with a DW_CFA_restore_state, parseInstructions can skip restore
+  // opcodes if it reaches the target PC and stops interpreting, so we have to
+  // make sure we don't leak memory.
+  while (rememberStack) {
+    PrologInfoStackEntry *next = rememberStack->next;
+    free(rememberStack);
+    rememberStack = next;
+  }
+
+  return returnValue;
 }
 
 /// "run" the DWARF instructions
 template <typename A>
 bool CFI_Parser<A>::parseInstructions(A &addressSpace, pint_t instructions,
                                       pint_t instructionsEnd,
                                       const CIE_Info &cieInfo, pint_t pcoffset,
                                       PrologInfoStackEntry *&rememberStack,