Fix different memory allocation issues (emscripten-core#13442)

* Fix different memory allocation issues:
1. In library.js, my earlier PR gated >>> 0 to CAN_ADDRESS_2GB builds only. That was troublesome, since if one does malloc(-1), it would no longer be caught (malloc(0xFFFFFFFFull) can never succeed). Restored the >>> 0.
2. Fix STACK_OVERFLOW_CHECK=2 mode to work with MINIMAL_RUNTIME. The call to ___set_stack_limits() was misplaced in MINIMAL_RUNTIME postamble.
3. Fix issue with sbrk() not being able to handle signed 32-bit integer overflow in sbrk limit growth, leading to erroneously reporting succeeding massive 2GB heap grow operations.
   This causes a breaking change to emscripten/heap.h emscripten_get_sbrk_ptr() signature, which will now return a uintptr_t instead of intptr_t. Not worried much since it will lead to clear build error, and there are extremely few users of that function.
4. Fix issues with passing ridiculously large (or small negative) alloc sizes to emmalloc functions (>0xFFFFFFC7u). Add assertions to validate memory overflows in emmalloc.
5. Fix a crash with emmalloc when attempting to malloc() memory when there are absolutely zero bytes available in the heap. (not a single free memory region left)
6. Add a test.
7. Misc comment updates.

* flake

* Fix validate_alloc_size

* Address review.

* Add dlmalloc_test_large.c

* Remove test line

* Separate to new test

src/library.js

@@ -251,9 +251,7 @@ LibraryManager.library = {
   ],
   emscripten_resize_heap: function(requestedSize) {
     var oldSize = HEAPU8.length;
-#if CAN_ADDRESS_2GB
     requestedSize = requestedSize >>> 0;
-#endif
 #if ALLOW_MEMORY_GROWTH == 0
 #if ABORTING_MALLOC
     abortOnCannotGrowMemory(requestedSize);

src/postamble_minimal.js

@@ -13,10 +13,6 @@ function run() {
   emscriptenMemoryProfiler.onPreloadComplete();
 #endif
 
-#if STACK_OVERFLOW_CHECK >= 2
-  ___set_stack_limits(_emscripten_stack_get_base(), _emscripten_stack_get_end());
-#endif
-
   <<< ATMAINS >>>
 
 #if PROXY_TO_PTHREAD
@@ -75,6 +71,9 @@ function initRuntime(asm) {
 #if STACK_OVERFLOW_CHECK
   _emscripten_stack_init();
   writeStackCookie();
+#if STACK_OVERFLOW_CHECK >= 2
+  ___set_stack_limits(_emscripten_stack_get_base(), _emscripten_stack_get_end());
+#endif
 #endif
 
 #if '___wasm_call_ctors' in IMPLEMENTED_FUNCTIONS

system/include/emscripten/emmalloc.h

@@ -44,6 +44,8 @@ void emscripten_builtin_free(void *ptr);
 // allocated with one of the emmalloc memory allocation functions (malloc, memalign, ...).
 // If called with size == 0, the pointer ptr is freed, and a null pointer is returned. If
 // called with null ptr, a new pointer is allocated.
+// If there is not enough memory, the old memory block is not freed and null pointer is
+// returned.
 void *realloc(void *ptr, size_t size);
 void *emmalloc_realloc(void *ptr, size_t size);
 

system/include/emscripten/heap.h

@@ -22,7 +22,7 @@ extern "C" {
 
 // Returns a pointer to a memory location that contains the heap DYNAMICTOP
 // variable (the end of the dynamic memory region)
-intptr_t *emscripten_get_sbrk_ptr(void);
+uintptr_t *emscripten_get_sbrk_ptr(void);
 
 // Attempts to geometrically or linearly increase the heap so that it
 // grows by at least requested_growth_bytes new bytes. The heap size may

system/lib/emmalloc.cpp

@@ -80,6 +80,10 @@ extern "C"
 // in the size field. I.e. for free regions, the size field is odd, and for used regions, the size field reads even.
 #define FREE_REGION_FLAG 0x1u
 
+// Attempts to malloc() more than this many bytes would cause an overflow when calculating the size of a region,
+// therefore allocations larger than this are short-circuited immediately on entry.
+#define MAX_ALLOC_SIZE 0xFFFFFFC7u
+
 // A free region has the following structure:
 // <size:uint32_t> <prevptr> <nextptr> ... <size:uint32_t>
 
@@ -633,8 +637,13 @@ static size_t validate_alloc_alignment(size_t alignment)
 
 static size_t validate_alloc_size(size_t size)
 {
+  assert(size + REGION_HEADER_SIZE > size);
+
   // Allocation sizes must be a multiple of pointer sizes, and at least 2*sizeof(pointer).
-  return size > 2*sizeof(Region*) ? (size_t)ALIGN_UP(size, sizeof(Region*)) : 2*sizeof(Region*);
+  size_t validatedSize = size > 2*sizeof(Region*) ? (size_t)ALIGN_UP(size, sizeof(Region*)) : 2*sizeof(Region*);
+  assert(validatedSize >= size); // 32-bit wraparound should not occur, too large sizes should be stopped before
+
+  return validatedSize;
 }
 
 static void *allocate_memory(size_t alignment, size_t size)
@@ -657,6 +666,14 @@ static void *allocate_memory(size_t alignment, size_t size)
     return 0;
   }
 
+  if (size > MAX_ALLOC_SIZE)
+  {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(console.log('Allocation failed: attempted allocation size is too large: ' + ($0 >>> 0) + 'bytes! (negative integer wraparound?)'), size);
+#endif
+    return 0;
+  }
+
   alignment = validate_alloc_alignment(alignment);
   size = validate_alloc_size(size);
 
@@ -720,11 +737,12 @@ static void *allocate_memory(size_t alignment, size_t size)
   // None of the buckets were able to accommodate an allocation. If this happens we are almost out of memory.
   // The largest bucket might contain some suitable regions, but we only looked at one region in that bucket, so
   // as a last resort, loop through more free regions in the bucket that represents the largest allocations available.
-  // But only if the bucket representing largest allocations available is not any of the first ten buckets (thirty buckets
-  // in 64-bit buckets build), these represent allocatable areas less than <1024 bytes - which could be a lot of scrap.
+  // But only if the bucket representing largest allocations available is not any of the first thirty buckets,
+  // these represent allocatable areas less than <1024 bytes - which could be a lot of scrap.
   // In such case, prefer to sbrk() in more memory right away.
   int largestBucketIndex = NUM_FREE_BUCKETS - 1 - __builtin_clzll(freeRegionBucketsUsed);
-  Region *freeRegion = freeRegionBuckets[largestBucketIndex].next;
+  // freeRegion will be null if there is absolutely no memory left. (all buckets are 100% used)
+  Region *freeRegion = freeRegionBucketsUsed ? freeRegionBuckets[largestBucketIndex].next : 0;
   if (freeRegionBucketsUsed >> 30)
   {
     // Look only at a constant number of regions in this bucket max, to avoid bad worst case behavior.
@@ -742,19 +760,23 @@ static void *allocate_memory(size_t alignment, size_t size)
 
   // We were unable to find a free memory region. Must sbrk() in more memory!
   size_t numBytesToClaim = size+sizeof(Region)*3;
+  assert(numBytesToClaim > size); // 32-bit wraparound should not happen here, allocation size has been validated above!
   bool success = claim_more_memory(numBytesToClaim);
   if (success)
     return allocate_memory(alignment, size); // Recurse back to itself to try again
 
   // also sbrk() failed, we are really really constrained :( As a last resort, go back to looking at the
   // bucket we already looked at above, continuing where the above search left off - perhaps there are
   // regions we overlooked the first time that might be able to satisfy the allocation.
-  while(freeRegion != &freeRegionBuckets[largestBucketIndex])
+  if (freeRegion)
   {
-    void *ptr = attempt_allocate(freeRegion, alignment, size);
-    if (ptr)
-      return ptr;
-    freeRegion = freeRegion->next;
+    while(freeRegion != &freeRegionBuckets[largestBucketIndex])
+    {
+      void *ptr = attempt_allocate(freeRegion, alignment, size);
+      if (ptr)
+        return ptr;
+      freeRegion = freeRegion->next;
+    }
   }
 
 #ifdef EMMALLOC_VERBOSE
@@ -986,11 +1008,17 @@ void *emmalloc_aligned_realloc(void *ptr, size_t alignment, size_t size)
     return 0;
   }
 
-  assert(IS_POWER_OF_2(alignment));
+  if (size > MAX_ALLOC_SIZE)
+  {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(console.log('Allocation failed: attempted allocation size is too large: ' + ($0 >>> 0) + 'bytes! (negative integer wraparound?)'), size);
+#endif
+    return 0;
+  }
 
+  assert(IS_POWER_OF_2(alignment));
   // aligned_realloc() cannot be used to ask to change the alignment of a pointer.
   assert(HAS_ALIGNMENT(ptr, alignment));
-
   size = validate_alloc_size(size);
 
   // Calculate the region start address of the original allocation
@@ -1013,6 +1041,8 @@ void *emmalloc_aligned_realloc(void *ptr, size_t alignment, size_t size)
     memcpy(newptr, ptr, MIN(size, region->size - REGION_HEADER_SIZE));
     free(ptr);
   }
+  // N.B. If there is not enough memory, the old memory block should not be freed and
+  // null pointer is returned.
   return newptr;
 }
 
@@ -1035,6 +1065,15 @@ void *emmalloc_realloc_try(void *ptr, size_t size)
     free(ptr);
     return 0;
   }
+
+  if (size > MAX_ALLOC_SIZE)
+  {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(console.log('Allocation failed: attempted allocation size is too large: ' + ($0 >>> 0) + 'bytes! (negative integer wraparound?)'), size);
+#endif
+    return 0;
+  }
+
   size = validate_alloc_size(size);
 
   // Calculate the region start address of the original allocation
@@ -1062,6 +1101,14 @@ void *emmalloc_aligned_realloc_uninitialized(void *ptr, size_t alignment, size_t
     return 0;
   }
 
+  if (size > MAX_ALLOC_SIZE)
+  {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(console.log('Allocation failed: attempted allocation size is too large: ' + ($0 >>> 0) + 'bytes! (negative integer wraparound?)'), size);
+#endif
+    return 0;
+  }
+
   size = validate_alloc_size(size);
 
   // Calculate the region start address of the original allocation

system/lib/sbrk.c

@@ -31,41 +31,42 @@
 
 extern size_t __heap_base;
 
-static intptr_t sbrk_val = (intptr_t)&__heap_base;
+static uintptr_t sbrk_val = (uintptr_t)&__heap_base;
 
-intptr_t* emscripten_get_sbrk_ptr() {
+uintptr_t* emscripten_get_sbrk_ptr() {
 #ifdef __PIC__
   // In relocatable code we may call emscripten_get_sbrk_ptr() during startup,
   // potentially *before* the setup of the dynamically-linked __heap_base, when
   // using SAFE_HEAP. (SAFE_HEAP instruments *all* memory accesses, so even the
   // code doing dynamic linking itself ends up instrumented, which is why we can
   // get such an instrumented call before sbrk_val has its proper value.)
   if (sbrk_val == 0) {
-    sbrk_val = (intptr_t)&__heap_base;
+    sbrk_val = (uintptr_t)&__heap_base;
   }
 #endif
   return &sbrk_val;
 }
 
-void *sbrk(intptr_t increment) {
+void *sbrk(intptr_t increment_) {
   uintptr_t old_size;
   // Enforce preserving a minimal 4-byte alignment for sbrk.
+  uintptr_t increment = (uintptr_t)increment_;
   increment = (increment + 3) & ~3;
 #if __EMSCRIPTEN_PTHREADS__
   // Our default dlmalloc uses locks around each malloc/free, so no additional
   // work is necessary to keep things threadsafe, but we also make sure sbrk
   // itself is threadsafe so alternative allocators work. We do that by looping
   // and retrying if we hit interference with another thread.
-  intptr_t expected;
+  uintptr_t expected;
   while (1) {
 #endif // __EMSCRIPTEN_PTHREADS__
-    intptr_t* sbrk_ptr = emscripten_get_sbrk_ptr();
+    uintptr_t* sbrk_ptr = emscripten_get_sbrk_ptr();
 #if __EMSCRIPTEN_PTHREADS__
-    intptr_t old_brk = __c11_atomic_load((_Atomic(intptr_t)*)sbrk_ptr, __ATOMIC_SEQ_CST);
+    uintptr_t old_brk = __c11_atomic_load((_Atomic(uintptr_t)*)sbrk_ptr, __ATOMIC_SEQ_CST);
 #else
-    intptr_t old_brk = *sbrk_ptr;
+    uintptr_t old_brk = *sbrk_ptr;
 #endif
-    intptr_t new_brk = old_brk + increment;
+    uintptr_t new_brk = old_brk + increment;
     // Check for a 32-bit overflow, which would indicate that we are trying to
     // allocate over 4GB, which is never possible in wasm32.
     if (increment > 0 && (uint32_t)new_brk <= (uint32_t)old_brk) {
@@ -84,7 +85,7 @@ void *sbrk(intptr_t increment) {
     // by iterating the loop body again.
     expected = old_brk;
     __c11_atomic_compare_exchange_strong(
-        (_Atomic(intptr_t)*)sbrk_ptr,
+        (_Atomic(uintptr_t)*)sbrk_ptr,
         &expected, new_brk,
         __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
     if (expected != old_brk) {
@@ -108,13 +109,13 @@ void *sbrk(intptr_t increment) {
   return (void*)-1;
 }
 
-int brk(intptr_t ptr) {
+int brk(uintptr_t ptr) {
 #if __EMSCRIPTEN_PTHREADS__
   // FIXME
   printf("brk() is not theadsafe yet, https://github.com/emscripten-core/emscripten/issues/10006");
   abort();
 #endif
-  intptr_t last = (intptr_t)sbrk(0);
+  uintptr_t last = (uintptr_t)sbrk(0);
   if (sbrk(ptr - last) == (void*)-1) {
     return -1;
   }

tests/browser/emmalloc_memgrowth.cpp

@@ -0,0 +1,41 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <emscripten/heap.h>
+
+uint64_t nextAllocationSize = 16*1024*1024;
+bool allocHasFailed = false;
+
+void grow_memory()
+{
+	uint8_t *ptr = (uint8_t*)malloc((size_t)nextAllocationSize);
+	EM_ASM({}, ptr); // Pass ptr out to confuse LLVM that it is used, so it won't optimize it away in -O1 and higher.
+	size_t heapSize = emscripten_get_heap_size();
+	printf("Allocated %zu: %d. Heap size: %zu\n", (size_t)nextAllocationSize, ptr ? 1 : 0, heapSize);
+	if (ptr)
+	{
+		if (!allocHasFailed)
+		{
+			nextAllocationSize *= 2;
+			// Make sure we don't overflow, and also exercise malloc(-1) to gracefully return 0 in ABORTING_MALLOC=0 mode.
+			if (nextAllocationSize > 0xFFFFFFFFULL)
+				nextAllocationSize = 0xFFFFFFFFULL;
+		}
+	}
+	else
+	{
+		nextAllocationSize /= 2;
+		allocHasFailed = true;
+	}
+}
+
+int main()
+{
+	// Exhaust all available memory.
+	for(int i = 0; i < 50; ++i)
+		grow_memory();
+	// If we get this far without crashing on OOM, we are ok!
+	printf("Test finished!\n");
+#ifdef REPORT_RESULT
+	REPORT_RESULT(0);
+#endif
+}

tests/code_size/hello_webgl2_wasm.json

@@ -3,8 +3,8 @@
   "a.html.gz": 377,
   "a.js": 4882,
   "a.js.gz": 2330,
-  "a.wasm": 10407,
-  "a.wasm.gz": 6689,
-  "total": 15852,
-  "total_gz": 9396
+  "a.wasm": 10448,
+  "a.wasm.gz": 6720,
+  "total": 15893,
+  "total_gz": 9427
 }

tests/code_size/hello_webgl2_wasm2js.json

@@ -1,10 +1,10 @@
 {
   "a.html": 588,
   "a.html.gz": 386,
-  "a.js": 20232,
-  "a.js.gz": 7961,
+  "a.js": 20432,
+  "a.js.gz": 8018,
   "a.mem": 3171,
   "a.mem.gz": 2715,
-  "total": 23991,
-  "total_gz": 11062
+  "total": 24191,
+  "total_gz": 11119
 }

tests/code_size/hello_webgl_wasm.json

@@ -3,8 +3,8 @@
   "a.html.gz": 377,
   "a.js": 4386,
   "a.js.gz": 2156,
-  "a.wasm": 10407,
-  "a.wasm.gz": 6689,
-  "total": 15356,
-  "total_gz": 9222
+  "a.wasm": 10448,
+  "a.wasm.gz": 6720,
+  "total": 15397,
+  "total_gz": 9253
 }

tests/code_size/hello_webgl_wasm2js.json

@@ -1,10 +1,10 @@
 {
   "a.html": 588,
   "a.html.gz": 386,
-  "a.js": 19717,
-  "a.js.gz": 7812,
+  "a.js": 19917,
+  "a.js.gz": 7870,
   "a.mem": 3171,
   "a.mem.gz": 2715,
-  "total": 23476,
-  "total_gz": 10913
+  "total": 23676,
+  "total_gz": 10971
 }

tests/core/test_emmalloc_trim.out

@@ -21,15 +21,15 @@ sbrk 2: 0x2902000
 3rd trim: 1
 dynamic heap 3: 33656832
 free dynamic memory 3: 102400
-unclaimed heap memory 3: 2108518400
-sbrk 3: 0x251a000
+unclaimed heap memory 3: 2104422400
+sbrk 3: 0x2902000
 4th trim: 0
 dynamic heap 4: 33656832
 free dynamic memory 4: 102400
-unclaimed heap memory 4: 2108518400
-sbrk 4: 0x251a000
+unclaimed heap memory 4: 2104422400
+sbrk 4: 0x2902000
 5th trim: 1
 dynamic heap 5: 33558528
 free dynamic memory 5: 0
-unclaimed heap memory 5: 2108616704
-sbrk 5: 0x2502000
+unclaimed heap memory 5: 2104422400
+sbrk 5: 0x2902000