Malloc fixes (#70)

* Several memory allocation fixes, primarily motivated by the fact that the Diffie-Hellman implementation in mbedtls is sensitive to misaligned allocations.  All malloc's are now 16-byte aligned.  This PR has several other points where remalloc was used instead of realloc, or memory needed to be zeroed upon allocation.  Finally, this PR also standardizes the definition of assert across layers, so that code in the lib directory can both use assertions and link properly in the PAL and shim.

LibOS/shim/src/shim_init.c

@@ -56,14 +56,20 @@ static void handle_failure (PAL_PTR event, PAL_NUM arg, PAL_CONTEXT * context)
     SHIM_GET_TLS()->pal_errno = (arg <= PAL_ERROR_BOUND) ? arg : 0;
 }
 
-void __assert_fail (const char * assertion, const char * file,
-                    unsigned int line, const char * function)
-{
-    __sys_printf("assert failed %s:%d %s\n", file, line, assertion);
+void __abort(void) {
     pause();
     shim_terminate();
 }
 
+void warn (const char *format, ...)
+{ 
+    va_list args;
+    va_start (args, format);
+    __sys_printf(format, args);
+    va_end (args);
+}
+
+
 void __stack_chk_fail (void)
 {
 }

LibOS/shim/test/apps/lmbench/lmbench-regression

@@ -120,7 +120,15 @@ function run {
 	TMPOUT=/tmp/OUT
 	rm -rf $TMPOUT
 	$LOADER "$@" 2>>$TMPOUT | tee -a $TMPOUT
-	cat $TMPOUT 1>&2
+	echo $?
+	echo "Done"
+	if [ $? -ne 0 ]
+	then
+	    cat $TMPOUT 1>&2
+	    exit 1
+	else
+	    cat $TMPOUT 1>&2
+	fi
 }
 
 date

LibOS/shim/test/regression/90_large-mmap.py

@@ -6,7 +6,7 @@ from regression import Regression
 loader = sys.argv[1]
 
 # Running Bootstrap
-regression = Regression(loader, "large-mmap", None, 60000)
+regression = Regression(loader, "large-mmap", None, 120000)
 
 regression.add_check(name="Ftruncate",
     check=lambda res: "large-mmap: ftruncate OK" in res[0].out)

Pal/lib/api.h

@@ -111,7 +111,7 @@ struct config_store {
     struct list_head root, entries;
     void *           raw_data;
     int              raw_size;
-    void *           (*malloc) (int);
+    void *           (*malloc) (size_t);
     void             (*free) (void *);
 };
 

Pal/lib/assert.h

@@ -0,0 +1,35 @@
+/* -*- mode:c; c-file-style:"k&r"; c-basic-offset: 4; tab-width:4; indent-tabs-mode:nil; mode:auto-fill; fill-column:78; -*- */
+/* vim: set ts=4 sw=4 et tw=78 fo=cqt wm=0: */
+
+/*
+ * assert.h
+ * 
+ * Define a common interface for assertions that builds for both the PAL 
+ * and libOS.
+ * 
+ */
+
+#ifndef ASSERT_H
+#define ASSERT_H
+
+/* All environments should implement warn, which prints a non-optional debug
+ * message. All environments should also implement __abort, which 
+ * terminates the process.
+ */
+
+void warn (const char *format, ...);
+void __abort(void);
+
+# define assert(test)                                                   \
+    ({                                                                  \
+        long _val = (long) (test);                                      \
+        (!(_val))                                                       \
+            ? ({                                                        \
+                    warn("assert failed " __FILE__ ":%d " #test " (value:%x)\n", \
+                         __LINE__, _val);                               \
+                    __abort(); })                                       \
+            : (void) 0;                                                 \
+    })
+
+#endif
+

Pal/lib/crypto/integer.c

@@ -33,9 +33,23 @@ void * malloc (int size);
 void free (void * mem);
 void * remalloc (const void * mem, int size);
 
+void memcpy(void *, void *, size_t);
+
 #define XMALLOC  malloc
 #define XFREE    free
-#define XREALLOC remalloc
+#define XREALLOC internal_realloc
+
+/* Implement realloc by ourselves, since the library doesn't provide realloc.
+ */
+void * internal_realloc(void *old, int old_size, int new_size)
+{
+    void *new = XMALLOC(new_size);
+    if (new == NULL)
+        return NULL;
+    memcpy(new, old, old_size);
+    XFREE(old);
+    return new;
+}
 
 static inline int toupper (int c)
 {
@@ -297,7 +311,10 @@ int mp_grow (mp_int * a, int size)
      * in case the operation failed we don't want
      * to overwrite the dp member of a.
      */
-    tmp = OPT_CAST(mp_digit) XREALLOC (a->dp, sizeof (mp_digit) * size);
+    //tmp = OPT_CAST(mp_digit) XREALLOC (a->dp, sizeof (mp_digit) * size);
+    tmp = OPT_CAST(mp_digit) internal_realloc(a->dp,
+                                              sizeof (mp_digit) * a->alloc,
+                                              sizeof (mp_digit) * size);
     if (tmp == NULL) {
       /* reallocation failed but "a" is still valid [can be freed] */
       return MP_MEM;

Pal/lib/crypto/mbedtls/mbedtls/config.h

@@ -1,5 +1,5 @@
 /* Copyright (C) 2017 Fortanix, Inc.
-
+   
    This file is part of Graphene Library OS.
 
    Graphene Library OS is free software: you can redistribute it and/or

Pal/lib/slabmgr.h

@@ -27,6 +27,7 @@
 #define SLABMGR_H
 
 #include "linux_list.h"
+#include <assert.h>
 #include <sys/mman.h>
 
 #ifndef system_malloc
@@ -42,20 +43,49 @@
 #define system_unlock() ({})
 #endif
 
-#define SLAB_PADDING   7
+/* malloc is supposed to provide some kind of alignment guarantees, but
+ * I can't find a specific reference to what that should be for x86_64.
+ * The first link here is a reference to a technical report from Mozilla,
+ * which seems to indicate that 64-bit platforms align return values to
+ * 16-bytes. calloc and malloc provide the same alignment guarantees.
+ * calloc additionally sets the memory to 0, which malloc is not required
+ * to do.
+ *
+ * http://www.erahm.org/2016/03/24/minimum-alignment-of-allocation-across-platforms/
+ * http://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html
+ */
+#define MIN_MALLOC_ALIGNMENT 16
+
+/* Slab objects need to be a multiple of 16 bytes to ensure proper address
+ * alignment for malloc and calloc. */
+#define OBJ_PADDING   15
+
+#define LARGE_OBJ_PADDING 8
+
+/* Returns the smallest exact multiple of _y that is at least as large as _x.
+ * In other words, returns _x if _x is a multiple of _y, otherwise rounds
+ * _x up to be a multiple of _y.
+ */
+#define ROUND_UP(_x, _y) ((((_x) + (_y) - 1) / (_y)) * (_y))
 
 typedef struct __attribute__((packed)) slab_obj {
     unsigned char level;
-    unsigned char padding[SLAB_PADDING];
+    unsigned char padding[OBJ_PADDING];
     union {
         struct list_head __list;
         unsigned char *raw;
     };
 } SLAB_OBJ_TYPE, * SLAB_OBJ;
 
+/* In order for slab elements to be 16-byte aligned, struct slab_area must
+ * be a multiple of 16 bytes. TODO: Add compile time assertion that this
+ * invariant is respected. */
+#define AREA_PADDING 12
+
 typedef struct __attribute__((packed)) slab_area {
     struct list_head __list;
     unsigned int size;
+    unsigned char pad[AREA_PADDING];
     unsigned char raw[];
 } SLAB_AREA_TYPE, * SLAB_AREA;
 
@@ -79,8 +109,10 @@ struct slab_debug {
 # define SLAB_CANARY_SIZE   0
 #endif
 
-#define SLAB_HDR_SIZE (sizeof(SLAB_OBJ_TYPE) - sizeof(struct list_head) + \
-                       SLAB_DEBUG_SIZE + SLAB_CANARY_SIZE)
+#define SLAB_HDR_SIZE \
+    ROUND_UP((sizeof(SLAB_OBJ_TYPE) - sizeof(struct list_head) +    \
+              SLAB_DEBUG_SIZE + SLAB_CANARY_SIZE), \
+             MIN_MALLOC_ALIGNMENT)
 
 #ifndef SLAB_LEVEL
 #define SLAB_LEVEL 8
@@ -110,9 +142,13 @@ typedef struct slab_mgr {
 } SLAB_MGR_TYPE, * SLAB_MGR;
 
 typedef struct __attribute__((packed)) large_mem_obj {
+    // offset 0
     unsigned long size;
+    unsigned char large_padding[LARGE_OBJ_PADDING];
+    // offset 16
     unsigned char level;
-    unsigned char padding[SLAB_PADDING];
+    unsigned char padding[OBJ_PADDING];
+    // offset 32
     unsigned char raw[];
 } LARGE_MEM_OBJ_TYPE, * LARGE_MEM_OBJ;
 
@@ -120,7 +156,7 @@ typedef struct __attribute__((packed)) large_mem_obj {
 #define OBJ_RAW(obj) (&(obj)->raw)
 
 #define RAW_TO_LEVEL(raw_ptr) \
-            (*((unsigned char *) (raw_ptr) - SLAB_PADDING - 1))
+            (*((unsigned char *) (raw_ptr) - OBJ_PADDING - 1))
 #define RAW_TO_OBJ(raw_ptr, type) container_of((raw_ptr), type, raw)
 
 #define __SUM_OBJ_SIZE(slab_size, size) \
@@ -346,6 +382,12 @@ static inline void * slab_alloc_debug (SLAB_MGR mgr, int size,
 
 static inline void slab_free (SLAB_MGR mgr, void * obj)
 {
+    /* In a general purpose allocator, free of NULL is allowed (and is a 
+     * nop). We might want to enforce stricter rules for our allocator if
+     * we're sure that no clients rely on being able to free NULL. */
+    if (obj == NULL)
+        return;
+    
     unsigned char level = RAW_TO_LEVEL(obj);
 
     if (level == (unsigned char) -1) {
@@ -354,8 +396,17 @@ static inline void slab_free (SLAB_MGR mgr, void * obj)
         return;
     }
 
-    if (level >= SLAB_LEVEL)
-        return;
+    /* If this happens, either the heap is already corrupted, or someone's
+     * freeing something that's wrong, which will most likely lead to heap
+     * corruption. Either way, panic if this happens. TODO: this doesn't allow
+     * us to detect cases where the heap headers have been zeroed, which
+     * is a common type of heap corruption. We could make this case slightly
+     * more likely to be detected by adding a non-zero offset to the level,
+     * so a level of 0 in the header would no longer be a valid level. */
+    if (level >= SLAB_LEVEL) {
+        pal_printf("Heap corruption detected: invalid heap level %ud\n", level);
+        assert(0); // panic
+    }
 
 #ifdef SLAB_CANARY
     unsigned long * m = (unsigned long *) (obj + slab_levels[level]);
@@ -375,6 +426,9 @@ static inline void slab_free (SLAB_MGR mgr, void * obj)
 static inline void slab_free_debug (SLAB_MGR mgr, void * obj,
                                     const char * file, int line)
 {
+    if (obj == NULL)
+        return;
+    
     unsigned char level = RAW_TO_LEVEL(obj);
 
     if (level < SLAB_LEVEL) {

Pal/src/db_exception.c

@@ -83,3 +83,17 @@ void DkExceptionReturn (PAL_PTR event)
 {
     _DkExceptionReturn(event);
 }
+
+/* This does not return */
+void __abort(void) {
+    _DkProcessExit(1);
+}
+
+void warn (const char *format, ...)
+{ 
+    va_list args;
+    va_start (args, format);
+    printf(format, args);
+    va_end (args);
+}
+

Pal/src/db_main.c

@@ -123,8 +123,11 @@ static void read_environments (const char *** envpp)
                 break;
             }
 
+    /* TODO: This code appears to rely on the memory buffer being zero-
+     * initialized, so we use calloc here to get zeroed memory. We should
+     * audit this code to verify that it's correct. */
     const char ** new_envp =
-            malloc(sizeof(const char *) * (nenvs + nsetenvs - noverwrite + 1));
+        calloc((nenvs + nsetenvs - noverwrite + 1), sizeof(const char *));
     memcpy(new_envp, envp, sizeof(const char *) * nenvs);
     envp = new_envp;
 

Pal/src/host/Linux-SGX/db_files.c

@@ -344,7 +344,9 @@ static int file_rename (PAL_HANDLE handle, const char * type,
     if (ret < 0)
         return ret;
 
-    handle->file.realpath = remalloc(uri, strlen(uri));
+    /* TODO: old realpath memory is potentially leaked here, and need
+     * to check for strdup memory allocation failure. */
+    handle->file.realpath = strdup(uri);
     return 0;
 }
 
@@ -532,7 +534,9 @@ static int dir_rename (PAL_HANDLE handle, const char * type,
     if (ret < 0)
         return ret;
 
-    handle->dir.realpath = remalloc(uri, strlen(uri));
+    /* TODO: old realpath memory is potentially leaked here, and need
+     * to check for strdup memory allocation failure. */
+    handle->dir.realpath = strdup(uri);
     return 0;
 }
 

Pal/src/host/Linux-SGX/db_main.c

@@ -105,6 +105,9 @@ static PAL_HANDLE setup_file_handle (const char * name, int fd)
     get_norm_path(name, path, 0, len + 1);
     handle->file.realpath = path;
 
+    handle->file.total = 0;
+    handle->file.stubs = NULL;
+
     return handle;
 }
 

Pal/src/host/Linux-SGX/enclave_untrusted.c

@@ -66,19 +66,10 @@ void init_untrusted_slab_mgr (int pagesize)
 
 void * malloc_untrusted (int size)
 {
-    void * ptr = slab_alloc(untrusted_slabmgr, size);
-
-    /* the slab manger will always remain at least one byte of padding,
-       so we can feel free to assign an offset at the byte prior to
-       the pointer */
-    if (ptr)
-        *(((unsigned char *) ptr) - 1) = 0;
-
-    return ptr;
+    return slab_alloc(untrusted_slabmgr, size);
 }
 
 void free_untrusted (void * ptr)
 {
-    ptr -= *(((unsigned char *) ptr) - 1);
     slab_free(untrusted_slabmgr, ptr);
 }

Pal/src/pal_internal.h

@@ -391,9 +391,10 @@ int add_elf_object(void * addr, PAL_HANDLE handle, int type);
 
 #ifndef NO_INTERNAL_ALLOC
 void init_slab_mgr (int alignment);
-void * malloc (int size);
-void * remalloc (const void * mem, int size);
-void * calloc (int nmem, int size);
+void * malloc (size_t size);
+void * remalloc (const void * mem, size_t size);
+void * calloc (size_t nmem, size_t size);
+char * strdup(const char *source);
 void free (void * mem);
 #endif
 

Pal/src/security/Linux/main.c

@@ -585,3 +585,17 @@ void do_main (void * args)
 exit:
     INLINE_SYSCALL(exit_group, 1, ret);
 }
+
+/* This does not return */
+void __abort(void) {
+    INLINE_SYSCALL(exit_group, 1, -1);
+}
+
+void warn (const char *format, ...)
+{ 
+    va_list args;
+    va_start (args, format);
+    printf(format, args);
+    va_end (args);
+}
+

Pal/src/slab.c

@@ -104,18 +104,19 @@ void init_slab_mgr (int alignment)
 #endif
 }
 
-void * malloc (int size)
+void * malloc (size_t size)
 {
 #if PROFILING == 1
     unsigned long before_slab = _DkSystemTimeQuery();
 #endif
     void * ptr = slab_alloc(slab_mgr, size);
 
-    /* the slab manger will always remain at least one byte of padding,
-       so we can feel free to assign an offset at the byte prior to
-       the pointer */
+#ifdef DEBUG
+    /* In debug builds, try to break code that uses uninitialized heap
+     * memory by explicitly initializing to a non-zero value. */
     if (ptr)
-        *(((unsigned char *) ptr) - 1) = 0;
+        memset(ptr, 0xa5, size);
+#endif
 
 #if PROFILING == 1
     pal_state.slab_time += _DkSystemTimeQuery() - before_slab;
@@ -123,7 +124,12 @@ void * malloc (int size)
     return ptr;
 }
 
-void * remalloc (const void * mem, int size)
+/* This function is not realloc(). remalloc() allocates a new buffer
+ * with with a provided size and copies the contents of the old buffer
+ * to the new buffer. The old buffer is not freed. The old buffer must
+ * be at least size bytes long. This function should probably be renamed
+ * to something less likely to be confused with realloc. */
+void * remalloc (const void * mem, size_t size)
 {
     void * nmem = malloc(size);
 
@@ -133,16 +139,23 @@ void * remalloc (const void * mem, int size)
     return nmem;
 }
 
-void * calloc (int nmem, int size)
+char * strdup (const char *s)
 {
-    void * ptr = malloc(nmem * size + size);
-    void * old_ptr = ptr;
+    size_t len = strlen(s) + 1;
+    char *new = malloc(len);
 
-    if (ptr) {
-        // align ptr to size
-        ptr = (char *) ptr + size - 1 - ((uintptr_t) ptr + size - 1) % size;
-        *(((char *) ptr) - 1) = (char *) ptr - (char *) old_ptr;
-    }
+    if (new)
+        memcpy(new, s, len);
+    
+    return new;
+}
+
+void * calloc (size_t nmem, size_t size)
+{
+    void * ptr = malloc(nmem * size);
+
+    if (ptr)
+        memset(ptr, 0, nmem * size);
 
     return ptr;
 }
@@ -152,8 +165,6 @@ void free (void * ptr)
 #if PROFILING == 1
     unsigned long before_slab = _DkSystemTimeQuery();
 #endif
-
-    ptr = (char *) ptr - *(((unsigned char *) ptr) - 1);
     slab_free(slab_mgr, ptr);
 
 #if PROFILING == 1