elf_add freeing strtab_view in fail, even though it shouldn't

[MeFisto94]

Hey,
I've stumbled over a crash in servo, which is using a rust wrapper around this library.
You can see my findings here, rust-lang/backtrace-rs#267, but a short description is: elf.c:elf_add#2965 claims, that "we hold on to the string table permanently.", which is only true until a elf.c:elf_add:fail#3197, which is caused by elf.c:elf_add#3071

Now I don't know if it's "okay" for L3071 to fail, like this expected, then Line 3197 is just wrong.
If this is a more critical fault or releasing the symnames there is required, then the changes made to the state (symdata) have to be rolled back, so noone is relying on them as the error is not propagated back, but skipped.

Thanks in Advance :)

[ianlancetaylor]

It does look like a bug but I also certainly don't expect the call to backtrace_get_view to fail. Why does it fail?

[MeFisto94]

It fails due to backtrace_get_view L77 -> file too short, size = 2884504395, got = 2147479552

[ianlancetaylor]

Can you show me the output of readelf -S --wide for the file in question? Thanks.

[MeFisto94]

I'll do that as soon as I'm back on my linux machine, thanks for the quick reply :)

[MeFisto94]

$ readelf -S --wide target/debug/servo
There are 47 section headers, starting at offset 0xbe4bd210:

Sektions-Header:
  [Nr] Name              Typ             Adresse          Off    Größe  ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .interp           PROGBITS        0000000000000270 000270 00001c 00   A  0   0  1
  [ 2] .note.ABI-tag     NOTE            000000000000028c 00028c 000020 00   A  0   0  4
  [ 3] .note.gnu.build-id NOTE            00000000000002ac 0002ac 000024 00   A  0   0  4
  [ 4] .dynsym           DYNSYM          00000000000002d0 0002d0 0141f0 18   A  5   1  8
  [ 5] .dynstr           STRTAB          00000000000144c0 0144c0 013d5b 00   A  0   0  1
  [ 6] .gnu.hash         GNU_HASH        0000000000028220 028220 000064 00   A  4   0  8
  [ 7] .gnu.version      VERSYM          0000000000028284 028284 001ad4 02   A  4   0  2
  [ 8] .gnu.version_r    VERNEED         0000000000029d58 029d58 0002f0 00   A  5  10  4
  [ 9] .rela.dyn         RELA            000000000002a048 02a048 c2c2c8 18   A  4   0  8
  [10] .rela.plt         RELA            0000000000c56310 c56310 0021c0 18  AI  4  12  8
  [11] .init             PROGBITS        0000000000c584d0 c584d0 000017 00  AX  0   0  4
  [12] .plt              PROGBITS        0000000000c584f0 c584f0 001690 10  AX  0   0 16
  [13] .text             PROGBITS        0000000000c59b80 c59b80 9d92d30 00  AX  0   0 16
  [14] .fini             PROGBITS        000000000a9ec8b0 a9ec8b0 000009 00  AX  0   0  4
  [15] .rodata           PROGBITS        000000000a9ec8c0 a9ec8c0 10364a3 00   A  0   0 64
  [16] .gcc_except_table PROGBITS        000000000ba22d64 ba22d64 5b30ec 00   A  0   0  4
  [17] .debug_gdb_scripts PROGBITS        000000000bfd5e50 bfd5e50 000022 01 AMS  0   0  1
  [18] .eh_frame         PROGBITS        000000000bfd5e78 bfd5e78 12ad554 00   A  0   0  8
  [19] .eh_frame_hdr     PROGBITS        000000000d2833cc d2833cc 4e8e44 00   A  0   0  4
  [20] .tdata            PROGBITS        000000000d76d580 d76c580 004b70 00 WAT  0   0 32
  [21] .tbss             NOBITS          000000000d772100 d771100 0007a0 00 WAT  0   0 32
  [22] .fini_array       FINI_ARRAY      000000000d772100 d771100 000008 08  WA  0   0  8
  [23] .init_array       INIT_ARRAY      000000000d772108 d771108 000158 00  WA  0   0  8
  [24] .data.rel.ro      PROGBITS        000000000d772260 d771260 2d77e0 00  WA  0   0 16
  [25] .dynamic          DYNAMIC         000000000da49a40 da48a40 0003d0 10  WA  5   0  8
  [26] .got              PROGBITS        000000000da49e10 da48e10 2bb688 00  WA  0   0  8
  [27] .got.plt          PROGBITS        000000000dd05498 dd04498 000b58 00  WA  0   0  8
  [28] .tm_clone_table   PROGBITS        000000000dd06000 dd05000 000000 00  WA  0   0  8
  [29] .data             PROGBITS        000000000dd06000 dd05000 020a39 00  WA  0   0 16
  [30] .bss              NOBITS          000000000dd26a40 dd25a39 22c312 00  WA  0   0 64
  [31] .comment          PROGBITS        0000000000000000 dd25a39 00009b 01  MS  0   0  1
  [32] .debug_str        PROGBITS        0000000000000000 dd25ad4 9d53f7a 01  MS  0   0  1
  [33] .debug_loc        PROGBITS        0000000000000000 17a79a4e 312a8e8 00      0   0  1
  [34] .debug_abbrev     PROGBITS        0000000000000000 1aba4336 58cc67 00      0   0  1
  [35] .debug_info       PROGBITS        0000000000000000 1b130f9d 410fa4a1 00      0   0  1
  [36] .debug_ranges     PROGBITS        0000000000000000 5c22b43e 2641550 00      0   0  1
  [37] .debug_macinfo    PROGBITS        0000000000000000 5e86c98e 003060 00      0   0  1
  [38] .debug_pubnames   PROGBITS        0000000000000000 5e86f9ee 99c0678 00      0   0  1
  [39] .debug_pubtypes   PROGBITS        0000000000000000 68230066 4ca72197 00      0   0  1
  [40] .debug_line       PROGBITS        0000000000000000 b4ca21fd 4f64422 00      0   0  1
  [41] .debug_aranges    PROGBITS        0000000000000000 b9c06620 0000c0 00      0   0 16
  [42] .debug_frame      PROGBITS        0000000000000000 b9c066e0 001290 00      0   0  8
  [43] .note.gnu.gold-version NOTE            0000000000000000 b9c07970 00001c 00      0   0  4
  [44] .symtab           SYMTAB          0000000000000000 b9c07990 134dd50 18     45 338232  8
  [45] .strtab           STRTAB          0000000000000000 baf556e0 356792d 00      0   0  1
  [46] .shstrtab         STRTAB          0000000000000000 be4bd00d 0001fe 00      0   0  1
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  l (large), p (processor specific)

Actually, it wants to read from debug_str, where size is reported as 9d53f7a from readelf, but we try to read a size of ABEE0B4B, which is much much more.

I guess the reason here is:

/* Read all the debug sections in a single view, since they are
     probably adjacent in the file.  We never release this view.  */

Which is just not true in this case. but would lead to further issues further down the road? Or would it just be "reading too much unneeded stuff, temporarly"?

I don't understand how end = sections[i].offset + sections[i].size; can "overflow" here, though. as .debug_frame + size ends at .note.gnu.gold-version and that max_offset is 0xB9C0661F, so exactly before _debug.frame.

NVM, I should learn to google suspicious numbers first :D
https://stackoverflow.com/questions/50302388/how-to-write-data-more-than-2g-in-a-time
It's simple: backtrace_get_view has to read more than these 2g, and those won't work at the same time, which is why reading is less than requested.

It's also probably unfeasible to read a whole 2GiB into the memory for debugging symbols.
Maybe all we need is some code to detect if all sections really are adjacent?

Edit: It's also trivial to make backtrace_get_view issue multiple read syscalls, but I think it's in 99.9% of the cases an indicate of a fault.

[MeFisto94]

@ianlancetaylor Any progress or ideas on that?

[ianlancetaylor]

I haven't had time to look.

[thanm]

It fails due to backtrace_get_view L77 -> file too short, size = 2884504395, got = 2147479552

The returned value seems suspicious to me -- it's 2^31 - 4096.

It is possible that the kernel is refusing to make a large mmap in this situation, e.g. https://www.kernel.org/doc/Documentation/vm/overcommit-accounting? What is /proc/sys/vm/overcommit_memory on your system (and/or CommitLimit setting in /proc/meminfo)?

[MeFisto94]

$cat /proc/sys/vm/overcommit_memory 
0
$ cat /proc/meminfo | grep CommitLimit
CommitLimit:    95454040 kB

But yeah, see above, write() and read() are capped at 2^31 - 4096 in that case.
We could work around that by modulo'ing those syscalls with that value, however I don't think that's a solution. Instead we should try to check if the sections really are continuous [and don't exceed 2GiB] and if they are not [which is the case here] then read them separately.

[ianlancetaylor]

It looks like you are running on a 32-bit system.

The debug sections are adjacent. They're just big. By my count libbacktrace is going to try to read everything from 0xdd25000 to 0xb9c07000, which is 0xabee2000 bytes, pretty close to what you see (0xABEE0B4B). I guess that on your 32-bit system mmap has a limit of 0x7ffff000, so it fails.

libbacktrace would probably work if your .debug_pubnames and .debug_pubtypes sections were not so large. Together they are 0x5643280f bytes, or roughly have what libbacktrace is trying to allocate. libbacktrace doesn't use those sections. For that matter, neither does gdb. Do you know why you have them, and what expects to read them?

In any case, can you see whether this patch fixes the problem? Thanks.

Index: elf.c
===================================================================
--- elf.c	(revision 280109)
+++ elf.c	(working copy)
@@ -2659,10 +2659,13 @@ elf_add (struct backtrace_state *state,
   uint32_t debugaltlink_buildid_size;
   off_t min_offset;
   off_t max_offset;
+  off_t debug_size;
   struct backtrace_view debug_view;
   int debug_view_valid;
   unsigned int using_debug_view;
   uint16_t *zdebug_table;
+  struct backtrace_view split_debug_view[DEBUG_MAX];
+  unsigned char split_debug_view_valid[DEBUG_MAX];
   struct elf_ppc64_opd_data opd_data, *opd;
   struct dwarf_sections dwarf_sections;
 
@@ -2687,6 +2690,7 @@ elf_add (struct backtrace_state *state,
   debugaltlink_buildid_data = NULL;
   debugaltlink_buildid_size = 0;
   debug_view_valid = 0;
+  memset (&split_debug_view_valid[0], 0, sizeof split_debug_view_valid);
   opd = NULL;
 
   if (!backtrace_get_view (state, descriptor, 0, sizeof ehdr, error_callback,
@@ -3131,6 +3135,7 @@ elf_add (struct backtrace_state *state,
 
   min_offset = 0;
   max_offset = 0;
+  debug_size = 0;
   for (i = 0; i < (int) DEBUG_MAX; ++i)
     {
       off_t end;
@@ -3142,6 +3147,7 @@ elf_add (struct backtrace_state *state,
 	  end = sections[i].offset + sections[i].size;
 	  if (end > max_offset)
 	    max_offset = end;
+	  debug_size += sections[i].size;
 	}
       if (zsections[i].size != 0)
 	{
@@ -3150,6 +3156,7 @@ elf_add (struct backtrace_state *state,
 	  end = zsections[i].offset + zsections[i].size;
 	  if (end > max_offset)
 	    max_offset = end;
+	  debug_size += zsections[i].size;
 	}
     }
   if (min_offset == 0 || max_offset == 0)
@@ -3159,11 +3166,45 @@ elf_add (struct backtrace_state *state,
       return 1;
     }
 
-  if (!backtrace_get_view (state, descriptor, min_offset,
-			   max_offset - min_offset,
-			   error_callback, data, &debug_view))
-    goto fail;
-  debug_view_valid = 1;
+  /* If the total debug section size is large, assume that there are
+     gaps between the sections, and read them individually.  */
+
+  if (max_offset - min_offset < 0x20000000
+      || max_offset - min_offset < debug_size + 0x10000)
+    {
+      if (!backtrace_get_view (state, descriptor, min_offset,
+			       max_offset - min_offset,
+			       error_callback, data, &debug_view))
+	goto fail;
+      debug_view_valid = 1;
+    }
+  else
+    {
+      memset (&split_debug_view[0], 0, sizeof split_debug_view);
+      for (i = 0; i < (int) DEBUG_MAX; ++i)
+	{
+	  struct debug_section_info *dsec;
+
+	  if (sections[i].size != 0)
+	    dsec = &sections[i];
+	  else if (zsections[i].size != 0)
+	    dsec = &zsections[i];
+	  else
+	    continue;
+
+	  if (!backtrace_get_view (state, descriptor, dsec->offset, dsec->size,
+				   error_callback, data, &split_debug_view[i]))
+	    goto fail;
+	  split_debug_view_valid[i] = 1;
+
+	  if (sections[i].size != 0)
+	    sections[i].data = ((const unsigned char *)
+				split_debug_view[i].data);
+	  else
+	    zsections[i].data = ((const unsigned char *)
+				 split_debug_view[i].data);
+	}
+    }
 
   /* We've read all we need from the executable.  */
   if (!backtrace_close (descriptor, error_callback, data))
@@ -3171,22 +3212,25 @@ elf_add (struct backtrace_state *state,
   descriptor = -1;
 
   using_debug_view = 0;
-  for (i = 0; i < (int) DEBUG_MAX; ++i)
+  if (debug_view_valid)
     {
-      if (sections[i].size == 0)
-	sections[i].data = NULL;
-      else
-	{
-	  sections[i].data = ((const unsigned char *) debug_view.data
-			      + (sections[i].offset - min_offset));
-	  ++using_debug_view;
-	}
+      for (i = 0; i < (int) DEBUG_MAX; ++i)
+	{
+	  if (sections[i].size == 0)
+	    sections[i].data = NULL;
+	  else
+	    {
+	      sections[i].data = ((const unsigned char *) debug_view.data
+				  + (sections[i].offset - min_offset));
+	      ++using_debug_view;
+	    }
 
-      if (zsections[i].size == 0)
-	zsections[i].data = NULL;
-      else
-	zsections[i].data = ((const unsigned char *) debug_view.data
-			     + (zsections[i].offset - min_offset));
+	  if (zsections[i].size == 0)
+	    zsections[i].data = NULL;
+	  else
+	    zsections[i].data = ((const unsigned char *) debug_view.data
+				 + (zsections[i].offset - min_offset));
+	}
     }
 
   /* Uncompress the old format (--compress-debug-sections=zlib-gnu).  */
@@ -3218,6 +3262,13 @@ elf_add (struct backtrace_state *state,
 	  sections[i].data = uncompressed_data;
 	  sections[i].size = uncompressed_size;
 	  sections[i].compressed = 0;
+
+	  if (split_debug_view_valid[i])
+	    {
+	      backtrace_release_view (state, &split_debug_view[i],
+				      error_callback, data);
+	      split_debug_view_valid[i] = 0;
+	    }
 	}
     }
 
@@ -3250,7 +3301,14 @@ elf_add (struct backtrace_state *state,
       sections[i].size = uncompressed_size;
       sections[i].compressed = 0;
 
-      --using_debug_view;
+      if (debug_view_valid)
+	--using_debug_view;
+      else if (split_debug_view_valid[i])
+	{
+	  backtrace_release_view (state, &split_debug_view[i],
+				  error_callback, data);
+	  split_debug_view_valid[i] = 0;
+	}
     }
 
   if (zdebug_table != NULL)
@@ -3297,6 +3355,12 @@ elf_add (struct backtrace_state *state,
     backtrace_release_view (state, &buildid_view, error_callback, data);
   if (debug_view_valid)
     backtrace_release_view (state, &debug_view, error_callback, data);
+  for (i = 0; i < (int) DEBUG_MAX; ++i)
+    {
+      if (split_debug_view_valid[i])
+	backtrace_release_view (state, &split_debug_view[i],
+				error_callback, data);
+    }
   if (opd)
     backtrace_release_view (state, &opd->view, error_callback, data);
   if (descriptor != -1)

[MeFisto94]

Give me a few days to test your patch, since I need to replace a few things to get it working again.
However I am running a 64 bit system, but since backtrace_get_view uses read, http://man7.org/linux/man-pages/man2/read.2.html#NOTES applies.

I can't tell you about the sections in special, other than that the project is a huge project built on rust, and that the rust language is known for huge binaries (even the optimized release build is huge, here I think 1GiB vs 3 GiB), and that it builds on top of LLVM, so lldb might interpret them when gdb does not. But I am not certain at all over this.

[ianlancetaylor]

On GNU/Linux systems backtrace_get_view does not use read, it uses mmap. If you are getting the version that uses read, something has gone wrong.

[ianlancetaylor]

@MeFisto94 Did you have a chance to try the patch above? Thanks.

[MeFisto94]

So as written in #32, the downstream rust crate has a simple compilation which lead to read being compiled no matter the system and as such mmap wasn't used.
I'll try the patch right now, sorry that it took me a month to schedule some time for it.
Currently I am facing the problem that lldb doesn't see the debug symbols for libbacktrace anymore, so I only see assembly

[MeFisto94]

As far as I can tell, this patch fixes the issues.
I cannot judge about the parts using zsection, as my code didn't have them, I guess my version is rather old, but the rest works flawlessly now.
No crash happens and the sections are read one by one. What would however happen if one section already exceeds the 2GiB?

Actually the backtraces get printed again as well :) Thanks for the patch!

[MeFisto94]

Just one more question:
The trigger conditions are now gone, but the issue itself that strtab_view is freed in goto fail still remains, right?

[ianlancetaylor]

Good point, should be fixed now.