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

[Jorge Gorbe Moya via Phabricator via llvm-commits]

jgorbe updated this revision to Diff 219207.
jgorbe added a comment.

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.


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

https://reviews.llvm.org/D66904

Files:
  libunwind/src/DwarfParser.hpp
  libunwind/test/remember_state_leak.pass.sh.s


Index: libunwind/test/remember_state_leak.pass.sh.s
===================================================================
--- /dev/null
+++ libunwind/test/remember_state_leak.pass.sh.s
@@ -0,0 +1,57 @@
+# REQUIRES: linux
+# RUN: %build
+# RUN: %run
+
+# The following main function is a translation of this code:
+#
+# int main() {
+#   asm(".cfi_remember_state\n\t");
+#   _Unwind_Exception exc;
+#   _Unwind_ForcedUnwind(&exc, callback, 0);
+#   asm(".cfi_restore_state\n\t");
+# }
+#
+# When unwinding, the CFI parser will stop parsing opcodes after the current PC,
+# so in this case the DW_CFA_restore_state opcode will never be processed and,
+# if the library doesn't clean up properly, the store allocated by
+# DW_CFA_remember_state will be leaked.
+#
+# This test will fail when linked with an asan-enabled libunwind if the
+# remembered state is leaked.
+
+.text
+
+callback:
+  xorl %eax, %eax
+  retq
+
+.globl main                    # -- Begin function main
+.p2align 4, 0x90
+.type main, at function
+main:                                   # @main
+.cfi_startproc
+# %bb.0:
+pushq %rbp
+.cfi_def_cfa_offset 16
+.cfi_offset %rbp, -16
+movq %rsp, %rbp
+.cfi_def_cfa_register %rbp
+subq $48, %rsp
+.cfi_remember_state
+xorl %eax, %eax
+movl %eax, %edx
+leaq -32(%rbp), %rdi
+movabsq $callback, %rsi
+callq _Unwind_ForcedUnwind
+.cfi_restore_state
+xorl %ecx, %ecx
+movl %eax, -36(%rbp)         # 4-byte Spill
+movl %ecx, %eax
+addq $48, %rsp
+popq %rbp
+.cfi_def_cfa %rsp, 8
+retq
+.Lfunc_end1:
+.size main, .Lfunc_end1-main
+.cfi_endproc
+                                        # -- End function
Index: libunwind/src/DwarfParser.hpp
===================================================================
--- libunwind/src/DwarfParser.hpp
+++ libunwind/src/DwarfParser.hpp
@@ -360,13 +360,25 @@
   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,
+                        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);
+
+  // 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66904.219207.patch
Type: text/x-patch
Size: 3294 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190907/8bfad814/attachment.bin>