Merge pull request #2468 from svaarala/fix-asan-leak

Fix realloc() leak with existing allocation, zero new size, and GC triggered before first realloc() attempt

Makefile

@@ -198,7 +198,8 @@ CCOPTS_DEBUG += -g -ggdb
 CLANG_CCOPTS_NONDEBUG = $(CCOPTS_NONDEBUG)
 CLANG_CCOPTS_NONDEBUG += -Wshorten-64-to-32
 CLANG_CCOPTS_NONDEBUG += -Wcomma
-#CLANG_CCOPTS_NONDEBUG += -fsanitize=undefined
+
+CLANG_CCOPTS_DEBUG = $(CCOPTS_DEBUG)
 
 GXXOPTS_SHARED = -pedantic -ansi -std=c++11 -fstrict-aliasing -Wall -Wextra -Wunused-result -Wunused-function
 GXXOPTS_SHARED += -DDUK_CMDLINE_PRINTALERT_SUPPORT
@@ -476,22 +477,22 @@ build/duk-clang: $(DUK_SOURCE_DEPS) | build prep/nondebug
 	@# Use -Wcast-align to trigger issues like: https://github.com/svaarala/duktape/issues/270
 	@# Use -Wshift-sign-overflow to trigger issues like: https://github.com/svaarala/duktape/issues/812
 	@# -Weverything
-	$(CLANG) -o $@ -Wcast-align -Wshift-sign-overflow -Iprep/nondebug $(CLANG_CCOPTS_NONDEBUG) prep/nondebug/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
+	$(CLANG) -o $@ $(CLANG_CCOPTS_NONDEBUG) -Wcast-align -Wshift-sign-overflow prep/nondebug/duktape.c -Iprep/nondebug $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
 	-@ls -l $@ && size $@
 build/duk-clang-asan: $(DUK_SOURCE_DEPS) | build prep/nondebug
 	# Binary fails to start with linenoise included, so add -UDUK_CMDLINE_FANCY to disable linenoise.
-	$(CLANG) -o $@ -Wcast-align -Wshift-sign-overflow -fsanitize=address -fno-omit-frame-pointer -Iprep/nondebug $(CLANG_CCOPTS_NONDEBUG) -UDUK_CMDLINE_FANCY prep/nondebug/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(CCLIBS)
+	$(CLANG) -o $@ $(CLANG_CCOPTS_NONDEBUG) -Wcast-align -Wshift-sign-overflow -fsanitize=address -fno-omit-frame-pointer -Iprep/nondebug $(CLANG_CCOPTS_DEBUG) -O0 -g -UDUK_CMDLINE_FANCY prep/nondebug/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(CCLIBS)
 	-@ls -l $@ && size $@
 build/duk-clang-ubsan: $(DUK_SOURCE_DEPS) | build prep/nondebug
-	$(CLANG) -o $@ -Wcast-align -Wshift-sign-overflow -fsanitize=undefined -Iprep/nondebug $(CLANG_CCOPTS_NONDEBUG) prep/nondebug/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
+	$(CLANG) -o $@ $(CLANG_CCOPTS_NONDEBUG) -Wcast-align -Wshift-sign-overflow -fsanitize=undefined prep/nondebug/duktape.c -Iprep/nondebug $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
 	-@ls -l $@ && size $@
 build/duk-perf-clang: $(DUK_SOURCE_DEPS) | build prep/nondebug-perf
-	$(CLANG) -o $@ -Wcast-align -Wshift-sign-overflow -Iprep/nondebug-perf $(CLANG_CCOPTS_NONDEBUG) prep/nondebug-perf/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
+	$(CLANG) -o $@ $(CLANG_CCOPTS_NONDEBUG) -Wcast-align -Wshift-sign-overflow -Iprep/nondebug-perf prep/nondebug-perf/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
 	-@ls -l $@ && size $@
 
 build/duk-fuzzilli: $(DUK_SOURCE_DEPS) | build prep/fuzz
 	# Target for fuzzilli.  Adds in the appropriate debug flags, without doing the debug prints.
-	$(CLANG) -O3 -o $@ -Wall -Wextra -Wcast-align -Wshift-sign-overflow -fsanitize=undefined -fsanitize-coverage=trace-pc-guard  -Iprep/fuzz $(CLANG_CCOPTS_DEBUG) prep/fuzz/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
+	$(CLANG) -O3 -o $@ $(CLANG_CCOPTS_DEBUG) -Wall -Wextra -Wcast-align -Wshift-sign-overflow -fsanitize=undefined -fsanitize-coverage=trace-pc-guard -Iprep/fuzz prep/fuzz/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(LINENOISE_SOURCES) $(CCLIBS)
 
 build/duk-g++: $(DUK_SOURCE_DEPS) | build prep/nondebug
 	$(GXX) -o $@ -Iprep/nondebug $(GXXOPTS_NONDEBUG) prep/nondebug/duktape.c $(DUKTAPE_CMDLINE_SOURCES) $(CCLIBS)

examples/cmdline/duk_cmdline.c

@@ -1373,7 +1373,6 @@ void __sanitizer_cov_reset_edgeguards(void) {
 	}
 }
 
-
 void __sanitizer_cov_trace_pc_guard_init(duk_uint32_t *start, duk_uint32_t *stop) {
 	/* Avoid duplicate initialization. */
 	if (start == stop || *start) {
@@ -1394,7 +1393,11 @@ void __sanitizer_cov_trace_pc_guard_init(duk_uint32_t *start, duk_uint32_t *stop
 		puts("[COV] no shared memory bitmap available, skipping");
 		__shmem = (struct shmem_data *) malloc(SHM_SIZE);
 	} else {
+#if defined(S_IRUSR) && defined(S_IWUSR)
+		int fd = shm_open(shm_key, O_RDWR, S_IRUSR | S_IWUSR);
+#else
 		int fd = shm_open(shm_key, O_RDWR, S_IREAD | S_IWRITE);
+#endif
 		if (fd <= -1) {
 			fprintf(stderr, "Failed to open shared memory region: %s\n", strerror(errno));
 			_exit(-1);

releases/releases.yaml

@@ -1385,3 +1385,4 @@ duktape_releases:
     - "Reformat source with clang-format-12 (GH-2416, GH-2421, GH-2423, GH-2424, GH-2425, GH-2426, GH-2427, GH-2428)"
     - "Fix DUK_USE_GET_RANDOM_DOUBLE() argument handling (GH-2432, GH-2435)"
     - "Fix memory unsafe behavior when valstack size limit hit in call setup (GH-2448, GH-2451)"
+    - "Fix a realloc() memory leak triggered when (1) previous allocation exists, (2) new realloc size is 0, (3) GC is triggered before first realloc() attempt (GH-2468)"

src-input/duk_heap_memory.c

@@ -192,13 +192,7 @@ DUK_LOCAL DUK_NOINLINE_PERF DUK_COLD void *duk__heap_mem_realloc_slowpath(duk_he
 	/* ptr may be NULL */
 	DUK_ASSERT_DISABLE(newsize >= 0);
 
-	/* Newsize was 0 and realloc() returned NULL, this has the semantics
-	 * of free(oldptr), i.e. memory was successfully freed.
-	 */
-	if (newsize == 0) {
-		DUK_DD(DUK_DDPRINT("zero size realloc in slow path, return NULL"));
-		return NULL;
-	}
+	/* Unlike for malloc(), zero size NULL result check happens at the call site. */
 
 	if (!duk__heap_suppress_debuglog(heap)) {
 		DUK_D(DUK_DPRINT("first realloc attempt failed or voluntary GC limit reached, attempt to gc and retry"));
@@ -233,7 +227,7 @@ DUK_LOCAL DUK_NOINLINE_PERF DUK_COLD void *duk__heap_mem_realloc_slowpath(duk_he
 
 		DUK_ASSERT(newsize > 0);
 		res = heap->realloc_func(heap->heap_udata, ptr, newsize);
-		if (res || newsize == 0) {
+		if (res != NULL || newsize == 0) {
 			if (!duk__heap_suppress_debuglog(heap)) {
 				DUK_D(DUK_DPRINT("duk_heap_mem_realloc() succeeded after gc (pass %ld), alloc size %ld",
 				                 (long) (i + 1),
@@ -258,7 +252,7 @@ DUK_INTERNAL DUK_INLINE_PERF DUK_HOT void *duk_heap_mem_realloc(duk_heap *heap,
 #if defined(DUK_USE_VOLUNTARY_GC)
 	/* Voluntary periodic GC (if enabled). */
 	if (DUK_UNLIKELY(--(heap)->ms_trigger_counter < 0)) {
-		goto slowpath;
+		goto gc_retry;
 	}
 #endif
 
@@ -270,24 +264,22 @@ DUK_INTERNAL DUK_INLINE_PERF DUK_HOT void *duk_heap_mem_realloc(duk_heap *heap,
 		DUK_DDD(DUK_DDDPRINT("gc torture enabled, pretend that first realloc attempt fails"));
 		res = NULL;
 		DUK_UNREF(res);
-		goto slowpath;
+		goto gc_retry;
 	}
 #endif
 
 	res = heap->realloc_func(heap->heap_udata, ptr, newsize);
-	if (DUK_LIKELY(res != NULL)) {
-		return res;
+	if (DUK_LIKELY(res != NULL) || newsize == 0) {
+		if (res != NULL && newsize == 0) {
+			DUK_DD(DUK_DDPRINT("first realloc attempt returned NULL for zero size realloc, accept and return NULL"));
 		}
-
-slowpath:
-
-	if (newsize == 0) {
-		DUK_DD(DUK_DDPRINT("first realloc attempt returned NULL for zero size realloc, use slow path to deal with it"));
+		return res;
 	} else {
-		if (!duk__heap_suppress_debuglog(heap)) {
-			DUK_D(DUK_DPRINT("first realloc attempt failed, attempt to gc and retry"));
-		}
+		goto gc_retry;
 	}
+	/* Never here. */
+
+gc_retry:
 	return duk__heap_mem_realloc_slowpath(heap, ptr, newsize);
 }
 
@@ -309,10 +301,7 @@ DUK_LOCAL DUK_NOINLINE_PERF DUK_COLD void *duk__heap_mem_realloc_indirect_slowpa
 	DUK_ASSERT(heap->realloc_func != NULL);
 	DUK_ASSERT_DISABLE(newsize >= 0);
 
-	if (newsize == 0) {
-		DUK_DD(DUK_DDPRINT("zero size indirect realloc in slow path, return NULL"));
-		return NULL;
-	}
+	/* Unlike for malloc(), zero size NULL result check happens at the call site. */
 
 	if (!duk__heap_suppress_debuglog(heap)) {
 		DUK_D(DUK_DPRINT("first indirect realloc attempt failed, attempt to gc and retry"));
@@ -365,9 +354,8 @@ DUK_LOCAL DUK_NOINLINE_PERF DUK_COLD void *duk__heap_mem_realloc_indirect_slowpa
 		 * The pointer being reallocated may change after every mark-and-sweep.
 		 */
 
-		DUK_ASSERT(newsize > 0);
 		res = heap->realloc_func(heap->heap_udata, cb(heap, ud), newsize);
-		if (res || newsize == 0) {
+		if (res != NULL || newsize == 0) {
 			if (!duk__heap_suppress_debuglog(heap)) {
 				DUK_D(DUK_DPRINT("duk_heap_mem_realloc_indirect() succeeded after gc (pass %ld), alloc size %ld",
 				                 (long) (i + 1),
@@ -394,7 +382,7 @@ DUK_INTERNAL DUK_INLINE_PERF DUK_HOT void *duk_heap_mem_realloc_indirect(duk_hea
 #if defined(DUK_USE_VOLUNTARY_GC)
 	/* Voluntary periodic GC (if enabled). */
 	if (DUK_UNLIKELY(--(heap)->ms_trigger_counter < 0)) {
-		goto slowpath;
+		goto gc_retry;
 	}
 #endif
 
@@ -406,25 +394,23 @@ DUK_INTERNAL DUK_INLINE_PERF DUK_HOT void *duk_heap_mem_realloc_indirect(duk_hea
 		DUK_DDD(DUK_DDDPRINT("gc torture enabled, pretend that first indirect realloc attempt fails"));
 		res = NULL;
 		DUK_UNREF(res);
-		goto slowpath;
+		goto gc_retry;
 	}
 #endif
 
 	res = heap->realloc_func(heap->heap_udata, cb(heap, ud), newsize);
-	if (DUK_LIKELY(res != NULL)) {
-		return res;
-	}
-
-slowpath:
-
-	if (newsize == 0) {
+	if (DUK_LIKELY(res != NULL) || newsize == 0) {
+		if (res != NULL && newsize == 0) {
 			DUK_DD(DUK_DDPRINT(
-		    "first indirect realloc attempt returned NULL for zero size realloc, use slow path to deal with it"));
-	} else {
-		if (!duk__heap_suppress_debuglog(heap)) {
-			DUK_D(DUK_DPRINT("first indirect realloc attempt failed, attempt to gc and retry"));
+			    "first indirect realloc attempt returned NULL for zero size realloc, accept and return NULL"));
 		}
+		return res;
+	} else {
+		goto gc_retry;
 	}
+	/* Never here. */
+
+gc_retry:
 	return duk__heap_mem_realloc_indirect_slowpath(heap, cb, ud, newsize);
 }
 

tests/shell/leak/asan_misc_expressions.sh

@@ -1,4 +1,4 @@
 #!/bin/sh
 
 make build/duk-clang-asan
-build/duk-clang-asan tests/ecmascript/test-misc-large-expressions.js
+ASAN_OPTIONS=fast_unwind_on_malloc=0 build/duk-clang-asan tests/ecmascript/test-misc-large-expressions.js