Re: Dumper issue, revisited; invalid realloc/free

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

On Wed, Feb 04, 2015 at 09:21:03PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 4 Feb 2015 12:57:09 -0500
> > From: Rich Felker <dalias@libc.org>
> > 
> > Last summer I started a thread about the ever-recurring dumper
> > portability problem and how it was blocking use of emacs on systems
> > based on musl libc. Recently I've been working with several people
> > interested in getting emacs working on Alpine Linux and musl-based
> > Gentoo, and I made some progress working around the issue:
> > 
> > http://www.openwall.com/lists/musl/2015/02/03/1
> 
> I suggest that you take a look at src/w32heap.c on Emacs's master
> branch.  There' you will see a simple solution of a very similar (if
> not identical) problem we have on MS-Windows.  It even includes a
> simple handling of large allocations.

As I suspected, this code is used only if you use gmalloc.c. It's not
used with system_malloc=yes, which is the case I'm concerned about.
> 
> > However, on further examination, the workaround I did is insufficient.
> > >From what I can tell, emacs is making an additional assumption on
> > malloc: not only that malloc results will be contiguous with
> > data/bss/brk so they can be dumped, but also that calling free() or
> > realloc() on these objects in the new process after dumping is valid.
> 
> Either that, or realloc/free are never called on the objects allocated
> before dumping.  On some platforms, the second assumption actually
> holds.

Actually that seemed to be true on 32-bit x86 for me (which is why I
originally thought my preload library was sufficient) but failed on
64-bit. All I can guess is that the larger pointer size perturbs the
behavior.

> > IMO this is utter nonsense, even with glibc or other widely-used
> > systems. It imposes an assumption that the heap structures in the
> > malloc version used at dump time match the heap structures in the
> > malloc version used at runtime, and that the runtime malloc is not
> > doing any sanity checks to catch and abort when a pointer into .data
> > is passed to realloc/free.
> 
> Or that the libc memory allocation routines can gracefully handle
> these situations.

I would not consider that "graceful". If they detect that the pointer
passed to realloc or free is invalid, the only reasonable behavior is
to abort. If they don't detect this case specially, then you're
relying on an assumption that they'll be compatible with the runtime
heap structures which could be invalidated by a libc upgrade. IMO the
only reasonable solution is for emacs to make sure it never calls free
or realloc with pointers that weren't obtained by malloc during the
same program invocation (i.e. as opposed to pre-dump malloc).

I'm attaching the patch. Apologies that it's not against latest emacs;
24.3 was the version I had lying around when I got started on this. At
this point it's just for comments/discussion. I'll regenerate it
against current emacs and clean it up if there's a possibility of it
actually getting upstreamed.

Rich

[-- Attachment #2: emacs_alloc_invalid_frees.diff --]
[-- Type: text/plain, Size: 1258 bytes --]

--- emacs-24.3.orig/src/alloc.c
+++ emacs-24.3/src/alloc.c
@@ -47,6 +47,13 @@
 
 #include <verify.h>
 
+static void *initial_brk;
+__attribute__((__constructor__))
+static void init()
+{
+	initial_brk = sbrk(0);
+}
+
 /* GC_CHECK_MARKED_OBJECTS means do sanity checks on allocated objects.
    Doable only if GC_MARK_STACK.  */
 #if ! GC_MARK_STACK
@@ -699,6 +706,14 @@
 {
   void *val;
 
+  if (block && block < initial_brk) {
+    size_t len = (char *)initial_brk - (char *)block;
+    if (len > size) len = size;
+    void *p = xmalloc(size);
+    memcpy(p, block, len);
+    return p;
+  }
+
   MALLOC_BLOCK_INPUT;
   /* We must call malloc explicitly when BLOCK is 0, since some
      reallocs don't do this.  */
@@ -720,6 +735,7 @@
 void
 xfree (void *block)
 {
+  if (block < initial_brk) return;
   if (!block)
     return;
   MALLOC_BLOCK_INPUT;
@@ -910,6 +926,7 @@
 static void
 lisp_free (void *block)
 {
+  if (block < initial_brk) return;
   MALLOC_BLOCK_INPUT;
   free (block);
 #if GC_MARK_STACK && !defined GC_MALLOC_CHECK
@@ -1117,6 +1134,8 @@
 {
   struct ablock *ablock = block;
   struct ablocks *abase = ABLOCK_ABASE (ablock);
+
+  if (block < initial_brk) return;
 
   MALLOC_BLOCK_INPUT;
 #if GC_MARK_STACK && !defined GC_MALLOC_CHECK