[PATCH] malloc tcache: Debugger now sees the address of the corrupted chunk.

[Adder]

On Thu, Dec 17, 2020 at 6:18 AM DJ Delorie <dj@redhat.com> wrote:
>
> Adder <adder.thief@gmail.com> writes:
> > But perhaps the speed cost is lower than it appears.
> > I have always been bad at evaluating time.
> > I am going to think about a way to benchmark this.
> > Suggestions are warmly welcome.
>
> https://developers.redhat.com/blog/2016/03/11/practical-micro-benchmarking-with-ltrace-and-sched/

Thank you ! I just need some time to start applying. I hope to get back on this.



> > In the meanwhile, could we also consider the following alternative ?
> >
> >   if (e_next)
> >     {
> >       if (__glibc_unlikely (e_next->key != tcache))
> >         please_crash_in_a_way_which_allows_debugger_to_print_e ();
> >     }
> >
> > Advantages (YMMV):
> >
> >   - no (volatile T *), no (void), no other cast;
> >   - easier to understand (even without "corrupted pointer" in mind);
> >   - only loads, no stores;
> >   - probability of e_next accidentally being valid and having a good key is low.
>
> Note that it's checking the next ptr in one chunk, and the key in a
> *different* chunk.

Affirmative.

We are validating e->next by looking at "REVEAL_PTR (e->next)->key".

This can *crash* if e->next points within a page we cannot read.

Otherwise, the test can still (correctly) *fail*
in a large number of cases when the pointer has been overwritten
(and just happens to REVEAL_PTR a readable page).

Finally, the test can also *pass*. (-:



> I think that's OK, corruption can happen anywhere,
> except that the please_crash would need to decide which 'e' is the
> relevent one for the message it's printing.

If a choice has to be made, I would strive to preserve the original 'e'.
Then the human debugger can walk the singly-linked list using gdb commands
(and pen-and-paper-or-more-gdb-magic to decode the encrypted pointers).

(The proposal below does not make a choice, it simply prints 4 pointers.
This is convenient for the human debugger,
at the expense of code bloat, albeit on the __glibc_unlikely path.)



> But I think we need to ask the larger glibc group if there's a preferred
> idiom for "make this variable available to the debugger"

Could we all have a look at malloc_printerr_ex below, please ?

I have tried other versions such as:

   __asm__ ("int $3");
   * (char *) NULL = 0;
   * (volatile const char *) NULL;

Either of these versions crashes as desired.

But the latter two (at least when factored out in a separate function)
cause tcache_get to miss out from "(gdb) backtrace".
And we cannot easily access "e" or any other locals for tcache_get.

We have to look at the CPU registers
(which are indeed better preserved than with malloc_printerr_ex).

This is a bit cumbersome since the code is far away from the main code
of tcache_get, which, by the way, cannot easily be reached any more
(now that tcache_get is missing out from the backtrace, methinks)
(e.g. "disas tcache_get" or "x/10i tcache_get" no longer works for me).

Please let me know if more details are useful regarding these other versions.



> > Disadvantages:
> >
> >   - conditional jump;
>
> That's what __glibc_unlikely() is for - it tells gcc to optimize the
> conditional for the common path, often negating the costs of the jump
> completely.

Indeed, I can now see that __glibc_unlikely does modify the machine code.
The conditional jump instruction points to the "unlikely" branch.
The "likely" branch simply follows after.

(FWIW, I have not personally benchmarked the result yet.)



> >> This is where malloc_printerr should be called.  Even if the data is
> >> corrupt, this is what we do elsewhere in these cases.
> >
> > Purpose is to give "e" to the debugger (including the human debugger).
> > In my testing (on x86_64), adding malloc_printerr here loses "e".
> >
> > For clarity and consistence with usage of malloc_printerr elsewhere,
> > I wish to suggest adding a function malloc_printerr_4
> > which is given the pointer to the string and 4 additional uint64_t args.
>
> I was going to suggest passing a chunk_ptr to malloc_printerr as that's
> what we usually have (or pass NULL if we don't).

Could we introduce a function which calls vsnprintf and malloc_printerr ?

We are going to be able to easily deploy it in other places where we find
that the debugger does not have access to important variables
(e.g. "corrupted size vs. prev_size", "corrupted double-linked list"
-- if so proven).

Behold a new proposal:

 static void malloc_printerr(const char *str) __attribute__ ((noreturn));
+static void malloc_printerr_ex(const char *format, ...) __attribute__
((noreturn));

 [...]

 static __always_inline void *
 tcache_get (size_t tc_idx)
 {
   tcache_entry *e = tcache->entries[tc_idx];
+
   if (__glibc_unlikely (!aligned_OK (e)))
     malloc_printerr ("malloc(): unaligned tcache chunk detected");
-  tcache->entries[tc_idx] = REVEAL_PTR (e->next);
+
+  tcache_entry *const e_next = REVEAL_PTR (e->next);
+
+  /* Validate e->next. It should point within another freed chunk (or
be NULL). */
+  if (e_next)
+    {
+      /* Test ability to read. Might succeed or crash or message-and-abort. */
+      if (__glibc_unlikely (e_next->key != tcache))
+        malloc_printerr_ex ("malloc tcache_get: Corrupted e->next"
+                            " (tcache %p, e %p, e_next %p) (e_next->key %p).",
+                            tcache, e, e_next, e_next->key);
+
+      /* Test ability to write. TODO: Benchmark, consider probability. */
+      ((volatile tcache_entry *) e_next)->key = e_next->key;
+    }
+
+  /* Validate e->key. */
+  if (__glibc_unlikely (e->key != tcache))
+    malloc_printerr_ex ("malloc tcache_get: Corrupted e->key"
+                        " (tcache %p, e %p, e_next %p) (e->key %p).",
+                        tcache, e, e_next, e->key);
+
+  tcache->entries[tc_idx] = e_next;
   --(tcache->counts[tc_idx]);
   e->key = NULL;
   return (void *) e;
 }

 [...]

+static void
+malloc_printerr_ex (const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+
+  char message [0x1000];
+  {
+    vsnprintf (message, sizeof (message) / sizeof (message [0]), format, ap);
+  }
+
+  va_end (ap);
+
+  malloc_printerr (message);
+}



> We have macros to convert tcache pointers to chunk pointers.

Thank you. I can now see mem2chunk and chunk2mem ! :)