Merge pull request #5951 from libgit2/ethomson/strict_alloc

Optional stricter allocation checking (for `malloc(0)` cases)

.github/workflows/main.yml

@@ -86,7 +86,7 @@ jobs:
           env:
             CC: gcc
             CMAKE_GENERATOR: Ninja
-            CMAKE_OPTIONS: -DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON
+            CMAKE_OPTIONS: -DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON -DDEBUG_STRICT_ALLOC=ON
           os: ubuntu-latest
         - # Xenial, GCC, mbedTLS
           container:

CMakeLists.txt

@@ -49,6 +49,7 @@ OPTION(USE_GSSAPI			"Link with libgssapi for SPNEGO auth"			OFF)
 OPTION(USE_STANDALONE_FUZZERS		"Enable standalone fuzzers (compatible with gcc)"	OFF)
 OPTION(USE_LEAK_CHECKER			"Run tests with leak checker"				OFF)
 OPTION(DEBUG_POOL			"Enable debug pool allocator"				OFF)
+OPTION(DEBUG_STRICT_ALLOC		"Enable strict allocator behavior"			OFF)
 OPTION(ENABLE_WERROR			"Enable compilation with -Werror"			OFF)
 OPTION(USE_BUNDLED_ZLIB    		"Use the bundled version of zlib. Can be set to one of Bundled(ON)/Chromium. The Chromium option requires a x86_64 processor with SSE4.2 and CLMUL"			OFF)
    SET(USE_HTTP_PARSER			"" CACHE STRING "Specifies the HTTP Parser implementation; either system or builtin.")

src/CMakeLists.txt

@@ -6,6 +6,11 @@ IF(DEBUG_POOL)
 ENDIF()
 ADD_FEATURE_INFO(debugpool GIT_DEBUG_POOL "debug pool allocator")
 
+IF(DEBUG_STRICT_ALLOC)
+	SET(GIT_DEBUG_STRICT_ALLOC 1)
+ENDIF()
+ADD_FEATURE_INFO(debugalloc GIT_DEBUG_STRICT_ALLOC "debug strict allocators")
+
 INCLUDE(PkgBuildConfig)
 INCLUDE(SanitizeBool)
 

src/allocators/stdalloc.c

@@ -9,34 +9,56 @@
 
 static void *stdalloc__malloc(size_t len, const char *file, int line)
 {
-	void *ptr = malloc(len);
+	void *ptr;
 
 	GIT_UNUSED(file);
 	GIT_UNUSED(line);
 
-	if (!ptr) git_error_set_oom();
+#ifdef GIT_DEBUG_STRICT_ALLOC
+	if (!len)
+		return NULL;
+#endif
+
+	ptr = malloc(len);
+
+	if (!ptr)
+		git_error_set_oom();
+
 	return ptr;
 }
 
 static void *stdalloc__calloc(size_t nelem, size_t elsize, const char *file, int line)
 {
-	void *ptr = calloc(nelem, elsize);
+	void *ptr;
 
 	GIT_UNUSED(file);
 	GIT_UNUSED(line);
 
-	if (!ptr) git_error_set_oom();
+#ifdef GIT_DEBUG_STRICT_ALLOC
+	if (!elsize || !nelem)
+		return NULL;
+#endif
+
+	ptr = calloc(nelem, elsize);
+
+	if (!ptr)
+		git_error_set_oom();
+
 	return ptr;
 }
 
 static char *stdalloc__strdup(const char *str, const char *file, int line)
 {
-	char *ptr = strdup(str);
+	char *ptr;
 
 	GIT_UNUSED(file);
 	GIT_UNUSED(line);
 
-	if (!ptr) git_error_set_oom();
+	ptr = strdup(str);
+
+	if (!ptr)
+		git_error_set_oom();
+
 	return ptr;
 }
 
@@ -48,7 +70,7 @@ static char *stdalloc__strndup(const char *str, size_t n, const char *file, int
 	length = p_strnlen(str, n);
 
 	if (GIT_ADD_SIZET_OVERFLOW(&alloclength, length, 1) ||
-		!(ptr = stdalloc__malloc(alloclength, file, line)))
+	    !(ptr = stdalloc__malloc(alloclength, file, line)))
 		return NULL;
 
 	if (length)
@@ -65,7 +87,7 @@ static char *stdalloc__substrdup(const char *start, size_t n, const char *file,
 	size_t alloclen;
 
 	if (GIT_ADD_SIZET_OVERFLOW(&alloclen, n, 1) ||
-		!(ptr = stdalloc__malloc(alloclen, file, line)))
+	    !(ptr = stdalloc__malloc(alloclen, file, line)))
 		return NULL;
 
 	memcpy(ptr, start, n);
@@ -75,12 +97,21 @@ static char *stdalloc__substrdup(const char *start, size_t n, const char *file,
 
 static void *stdalloc__realloc(void *ptr, size_t size, const char *file, int line)
 {
-	void *new_ptr = realloc(ptr, size);
+	void *new_ptr;
 
 	GIT_UNUSED(file);
 	GIT_UNUSED(line);
 
-	if (!new_ptr) git_error_set_oom();
+#ifdef GIT_DEBUG_STRICT_ALLOC
+	if (!size)
+		return NULL;
+#endif
+
+	new_ptr = realloc(ptr, size);
+
+	if (!new_ptr)
+		git_error_set_oom();
+
 	return new_ptr;
 }
 

src/features.h.in

@@ -2,6 +2,8 @@
 #define INCLUDE_features_h__
 
 #cmakedefine GIT_DEBUG_POOL 1
+#cmakedefine GIT_DEBUG_STRICT_ALLOC 1
+
 #cmakedefine GIT_TRACE 1
 #cmakedefine GIT_THREADS 1
 #cmakedefine GIT_WIN32_LEAKCHECK 1

src/merge.c

@@ -1535,7 +1535,8 @@ int git_merge_diff_list__find_renames(
 	GIT_ASSERT_ARG(diff_list);
 	GIT_ASSERT_ARG(opts);
 
-	if ((opts->flags & GIT_MERGE_FIND_RENAMES) == 0)
+	if ((opts->flags & GIT_MERGE_FIND_RENAMES) == 0 ||
+	    !diff_list->conflicts.length)
 		return 0;
 
 	similarity_ours = git__calloc(diff_list->conflicts.length,

src/pack-objects.c

@@ -517,13 +517,18 @@ static int cb_tag_foreach(const char *name, git_oid *oid, void *data)
 	return 0;
 }
 
-static git_pobject **compute_write_order(git_packbuilder *pb)
+static int compute_write_order(git_pobject ***out, git_packbuilder *pb)
 {
 	size_t i, wo_end, last_untagged;
 	git_pobject **wo;
 
+	*out = NULL;
+
+	if (!pb->nr_objects)
+		return 0;
+
 	if ((wo = git__mallocarray(pb->nr_objects, sizeof(*wo))) == NULL)
-		return NULL;
+		return -1;
 
 	for (i = 0; i < pb->nr_objects; i++) {
 		git_pobject *po = pb->object_list + i;
@@ -552,7 +557,7 @@ static git_pobject **compute_write_order(git_packbuilder *pb)
 	 */
 	if (git_tag_foreach(pb->repo, &cb_tag_foreach, pb) < 0) {
 		git__free(wo);
-		return NULL;
+		return -1;
 	}
 
 	/*
@@ -609,10 +614,11 @@ static git_pobject **compute_write_order(git_packbuilder *pb)
 	if (wo_end != pb->nr_objects) {
 		git__free(wo);
 		git_error_set(GIT_ERROR_INVALID, "invalid write order");
-		return NULL;
+		return -1;
 	}
 
-	return wo;
+	*out = wo;
+	return 0;
 }
 
 static int write_pack(git_packbuilder *pb,
@@ -625,15 +631,15 @@ static int write_pack(git_packbuilder *pb,
 	struct git_pack_header ph;
 	git_oid entry_oid;
 	size_t i = 0;
-	int error = 0;
+	int error;
 
-	write_order = compute_write_order(pb);
-	if (write_order == NULL)
-		return -1;
+	if ((error = compute_write_order(&write_order, pb)) < 0)
+		return error;
 
 	if (!git__is_uint32(pb->nr_objects)) {
 		git_error_set(GIT_ERROR_INVALID, "too many objects");
-		return -1;
+		error = -1;
+		goto done;
 	}
 
 	/* Write pack header */