Fix valgrind issues and leaks

This fixes up a number of problems flagged by valgrind and also
cleans up the internal `git_submodule` allocation handling
overall with a simpler model.

src/buffer.c

@@ -144,31 +144,40 @@ int git_buf_puts(git_buf *buf, const char *string)
 int git_buf_puts_escaped(
 	git_buf *buf, const char *string, const char *esc_chars, const char *esc_with)
 {
-	const char *scan = string;
+	const char *scan;
-	size_t total = 0, esc_with_len = strlen(esc_with);
+	size_t total = 0, esc_len = strlen(esc_with), count;
 
-	while (*scan) {
+	if (!string)
-		size_t count = strcspn(scan, esc_chars);
+		return 0;
-		total += count + 1 + esc_with_len;
+
-		scan += count + 1;
+	for (scan = string; *scan; ) {
+		/* count run of non-escaped characters */
+		count = strcspn(scan, esc_chars);
+		total += count;
+		scan += count;
+		/* count run of escaped characters */
+		count = strspn(scan, esc_chars);
+		total += count * (esc_len + 1);
+		scan += count;
 	}
 
 	ENSURE_SIZE(buf, buf->size + total + 1);
 
 	for (scan = string; *scan; ) {
-		size_t count = strcspn(scan, esc_chars);
+		count = strcspn(scan, esc_chars);
 
 		memmove(buf->ptr + buf->size, scan, count);
 		scan += count;
 		buf->size += count;
 
-		if (*scan) {
+		for (count = strspn(scan, esc_chars); count > 0; --count) {
-			memmove(buf->ptr + buf->size, esc_with, esc_with_len);
+			/* copy escape sequence */
-			buf->size += esc_with_len;
+			memmove(buf->ptr + buf->size, esc_with, esc_len);
-
+			buf->size += esc_len;
-			memmove(buf->ptr + buf->size, scan, 1);
+			/* copy character to be escaped */
-			scan += 1;
+			buf->ptr[buf->size] = *scan;
-			buf->size += 1;
+			buf->size++;
+			scan++;
 		}
 	}
 

src/config_file.c

@@ -195,7 +195,7 @@ static int file_foreach(
 	void *data)
 {
 	diskfile_backend *b = (diskfile_backend *)backend;
-	cvar_t *var;
+	cvar_t *var, *next_var;
 	const char *key;
 	regex_t regex;
 	int result = 0;
@@ -212,7 +212,9 @@ static int file_foreach(
 	}
 
 	git_strmap_foreach(b->values, key, var,
-		for (; var != NULL; var = CVAR_LIST_NEXT(var)) {
+		for (; var != NULL; var = next_var) {
+			next_var = CVAR_LIST_NEXT(var);
+
 			/* skip non-matching keys if regexp was provided */
 			if (regexp && regexec(&regex, key, 0, NULL, 0) != 0)
 				continue;

src/fileops.c

@@ -700,6 +700,7 @@ int git_futils_cp_r(
 	error = _cp_r_callback(&info, &path);
 
 	git_buf_free(&path);
+	git_buf_free(&info.to);
 
 	return error;
 }

src/submodule.c

@@ -71,10 +71,8 @@ static git_config_file *open_gitmodules(
 	git_repository *, bool, const git_oid *);
 static int lookup_head_remote(
 	git_buf *url, git_repository *repo);
-static git_submodule *submodule_lookup_or_create(
+static int submodule_get(
-	git_repository *repo, const char *n1, const char *n2);
+	git_submodule **, git_repository *, const char *, const char *);
-static int submodule_update_map(
-	git_repository *repo, git_submodule *sm, const char *key);
 static void submodule_release(
 	git_submodule *sm, int decr);
 static int submodule_load_from_index(
@@ -311,18 +309,9 @@ int git_submodule_add_setup(
 
 	/* add submodule to hash and "reload" it */
 
-	if ((sm = submodule_lookup_or_create(repo, path, NULL)) == NULL) {
+	if (!(error = submodule_get(&sm, repo, path, NULL)) &&
-		error = -1;
+		!(error = git_submodule_reload(sm)))
-		goto cleanup;
+		error = git_submodule_init(sm, false);
-	}
-
-	if ((error = submodule_update_map(repo, sm, sm->path)) < 0)
-		goto cleanup;
-
-	if ((error = git_submodule_reload(sm)) < 0)
-		goto cleanup;
-
-	error = git_submodule_init(sm, false);
 
 cleanup:
 	if (submodule != NULL)
@@ -757,6 +746,7 @@ int git_submodule_reload(git_submodule *submodule)
 				mods, path.ptr, submodule_load_from_config, repo);
 
 		git_buf_free(&path);
+		git_config_file_free(mods);
 	}
 
 	return error;
@@ -768,6 +758,9 @@ int git_submodule_status(
 {
 	assert(status && submodule);
 
+	GIT_UNUSED(status);
+	GIT_UNUSED(submodule);
+
 	/* TODO: move status code from below and update */
 
 	*status = 0;
@@ -781,19 +774,29 @@ int git_submodule_status(
 
 static git_submodule *submodule_alloc(git_repository *repo, const char *name)
 {
-	git_submodule *sm = git__calloc(1, sizeof(git_submodule));
+	git_submodule *sm;
-	if (sm == NULL)
-		return sm;
 
-	sm->path = sm->name = git__strdup(name);
+	if (!name || !strlen(name)) {
-	if (!sm->name) {
+		giterr_set(GITERR_SUBMODULE, "Invalid submodule name");
-		git__free(sm);
 		return NULL;
 	}
 
+	sm = git__calloc(1, sizeof(git_submodule));
+	if (sm == NULL)
+		goto fail;
+
+	sm->path = sm->name = git__strdup(name);
+	if (!sm->name)
+		goto fail;
+
 	sm->owner = repo;
+	sm->refcount = 1;
 
 	return sm;
+
+fail:
+	submodule_release(sm, 0);
+	return NULL;
 }
 
 static void submodule_release(git_submodule *sm, int decr)
@@ -821,54 +824,56 @@ static void submodule_release(git_submodule *sm, int decr)
 	}
 }
 
-static git_submodule *submodule_lookup_or_create(
+static int submodule_get(
-	git_repository *repo, const char *n1, const char *n2)
+	git_submodule **sm_ptr,
+	git_repository *repo,
+	const char *name,
+	const char *alternate)
 {
 	git_strmap *smcfg = repo->submodules;
 	khiter_t pos;
 	git_submodule *sm;
-
-	assert(n1);
-
-	pos = git_strmap_lookup_index(smcfg, n1);
-
-	if (!git_strmap_valid_index(smcfg, pos) && n2)
-		pos = git_strmap_lookup_index(smcfg, n2);
-
-	if (!git_strmap_valid_index(smcfg, pos))
-		sm = submodule_alloc(repo, n1);
-	else
-		sm = git_strmap_value_at(smcfg, pos);
-
-	return sm;
-}
-
-static int submodule_update_map(
-	git_repository *repo, git_submodule *sm, const char *key)
-{
-	void *old_sm;
 	int error;
 
-	git_strmap_insert2(repo->submodules, key, sm, old_sm, error);
+	assert(repo && name);
-	if (error < 0) {
+
-		submodule_release(sm, 0);
+	pos = git_strmap_lookup_index(smcfg, name);
-		return -1;
+
+	if (!git_strmap_valid_index(smcfg, pos) && alternate)
+		pos = git_strmap_lookup_index(smcfg, alternate);
+
+	if (!git_strmap_valid_index(smcfg, pos)) {
+		sm = submodule_alloc(repo, name);
+
+		/* insert value at name - if another thread beats us to it, then use
+		 * their record and release our own.
+		 */
+		pos = kh_put(str, smcfg, name, &error);
+
+		if (error < 0) {
+			submodule_release(sm, 1);
+			sm = NULL;
+		} else if (error == 0) {
+			submodule_release(sm, 1);
+			sm = git_strmap_value_at(smcfg, pos);
+		} else {
+			git_strmap_set_value_at(smcfg, pos, sm);
+		}
+	} else {
+		sm = git_strmap_value_at(smcfg, pos);
 	}
 
-	sm->refcount++;
+	*sm_ptr = sm;
 
-	if (old_sm && ((git_submodule *)old_sm) != sm)
+	return (sm != NULL) ? 0 : -1;
-		submodule_release(old_sm, 1);
-
-	return 0;
 }
 
 static int submodule_load_from_index(
 	git_repository *repo, const git_index_entry *entry)
 {
-	git_submodule *sm = submodule_lookup_or_create(repo, entry->path, NULL);
+	git_submodule *sm;
 
-	if (!sm)
+	if (submodule_get(&sm, repo, entry->path, NULL) < 0)
 		return -1;
 
 	if (sm->flags & GIT_SUBMODULE_STATUS_IN_INDEX) {
@@ -881,15 +886,15 @@ static int submodule_load_from_index(
 	git_oid_cpy(&sm->index_oid, &entry->oid);
 	sm->flags |= GIT_SUBMODULE_STATUS__INDEX_OID_VALID;
 
-	return submodule_update_map(repo, sm, sm->path);
+	return 0;
 }
 
 static int submodule_load_from_head(
 	git_repository *repo, const char *path, const git_oid *oid)
 {
-	git_submodule *sm = submodule_lookup_or_create(repo, path, NULL);
+	git_submodule *sm;
 
-	if (!sm)
+	if (submodule_get(&sm, repo, path, NULL) < 0)
 		return -1;
 
 	sm->flags |= GIT_SUBMODULE_STATUS_IN_HEAD;
@@ -897,7 +902,14 @@ static int submodule_load_from_head(
 	git_oid_cpy(&sm->head_oid, oid);
 	sm->flags |= GIT_SUBMODULE_STATUS__HEAD_OID_VALID;
 
-	return submodule_update_map(repo, sm, sm->path);
+	return 0;
+}
+
+static int submodule_config_error(const char *property, const char *value)
+{
+	giterr_set(GITERR_INVALID,
+		"Invalid value for submodule '%s' property: '%s'", property, value);
+	return -1;
 }
 
 static int submodule_load_from_config(
@@ -905,13 +917,11 @@ static int submodule_load_from_config(
 {
 	git_repository *repo = data;
 	git_strmap *smcfg = repo->submodules;
-	const char *namestart;
+	const char *namestart, *property, *alternate = NULL;
-	const char *property;
 	git_buf name = GIT_BUF_INIT;
 	git_submodule *sm;
-	void *old_sm = NULL;
 	bool is_path;
-	int error;
+	int error = 0;
 
 	if (git__prefixcmp(key, "submodule.") != 0)
 		return 0;
@@ -926,38 +936,45 @@ static int submodule_load_from_config(
 	if (git_buf_set(&name, namestart, property - namestart - 1) < 0)
 		return -1;
 
-	sm = submodule_lookup_or_create(repo, name.ptr, is_path ? value : NULL);
+	if (submodule_get(&sm, repo, name.ptr, is_path ? value : NULL) < 0) {
-	if (!sm)
+		git_buf_free(&name);
-		goto fail;
+		return -1;
+	}
 
 	sm->flags |= GIT_SUBMODULE_STATUS_IN_CONFIG;
 
+	/* Only from config might we get differing names & paths.  If so, then
+	 * update the submodule and insert under the alternative key.
+	 */
+
+	/* TODO: if case insensitive filesystem, then the following strcmps
+	 * should be strcasecmp
+	 */
+
 	if (strcmp(sm->name, name.ptr) != 0) {
-		assert(sm->path == sm->name);
+		alternate = sm->name = git_buf_detach(&name);
-		sm->name = git_buf_detach(&name);
+	} else if (is_path && value && strcmp(sm->path, value) != 0) {
-
+		alternate = sm->path = git__strdup(value);
-		git_strmap_insert2(smcfg, sm->name, sm, old_sm, error);
+		if (!sm->path)
-		if (error < 0)
+			error = -1;
-			goto fail;
-		sm->refcount++;
 	}
-	else if (is_path && value && strcmp(sm->path, value) != 0) {
+	if (alternate) {
-		assert(sm->path == sm->name);
+		void *old_sm = NULL;
-		sm->path = git__strdup(value);
+		git_strmap_insert2(smcfg, alternate, sm, old_sm, error);
-		if (sm->path == NULL)
-			goto fail;
 
-		git_strmap_insert2(smcfg, sm->path, sm, old_sm, error);
+		if (error >= 0)
-		if (error < 0)
+			sm->refcount++; /* inserted under a new key */
-			goto fail;
+
-		sm->refcount++;
+		/* if we replaced an old module under this key, release the old one */
+		if (old_sm && ((git_submodule *)old_sm) != sm) {
+			submodule_release(old_sm, 1);
+			/* TODO: log warning about multiple submodules with same path */
+		}
 	}
+
 	git_buf_free(&name);
-
+	if (error < 0)
-	if (old_sm && ((git_submodule *)old_sm) != sm) {
+		return error;
-		/* TODO: log warning about multiple submodules with same path */
-		submodule_release(old_sm, 1);
-	}
 
 	/* TODO: Look up path in index and if it is present but not a GITLINK
 	 * then this should be deleted (at least to match git's behavior)
@@ -968,48 +985,33 @@ static int submodule_load_from_config(
 
 	/* copy other properties into submodule entry */
 	if (strcasecmp(property, "url") == 0) {
-		if (sm->url) {
+		git__free(sm->url);
-			git__free(sm->url);
+		sm->url = NULL;
-			sm->url = NULL;
+
-		}
 		if (value != NULL && (sm->url = git__strdup(value)) == NULL)
-			goto fail;
+			return -1;
 	}
 	else if (strcasecmp(property, "update") == 0) {
 		int val;
 		if (git_config_lookup_map_value(
-			_sm_update_map, ARRAY_SIZE(_sm_update_map), value, &val) < 0) {
+			_sm_update_map, ARRAY_SIZE(_sm_update_map), value, &val) < 0)
-			giterr_set(GITERR_INVALID,
+			return submodule_config_error("update", value);
-				"Invalid value for submodule update property: '%s'", value);
-			goto fail;
-		}
 		sm->update_default = sm->update = (git_submodule_update_t)val;
 	}
 	else if (strcasecmp(property, "fetchRecurseSubmodules") == 0) {
-		if (git__parse_bool(&sm->fetch_recurse, value) < 0) {
+		if (git__parse_bool(&sm->fetch_recurse, value) < 0)
-			giterr_set(GITERR_INVALID,
+			return submodule_config_error("fetchRecurseSubmodules", value);
-				"Invalid value for submodule 'fetchRecurseSubmodules' property: '%s'", value);
-			goto fail;
-		}
 	}
 	else if (strcasecmp(property, "ignore") == 0) {
 		int val;
 		if (git_config_lookup_map_value(
-			_sm_ignore_map, ARRAY_SIZE(_sm_ignore_map), value, &val) < 0) {
+			_sm_ignore_map, ARRAY_SIZE(_sm_ignore_map), value, &val) < 0)
-			giterr_set(GITERR_INVALID,
+			return submodule_config_error("ignore", value);
-				"Invalid value for submodule ignore property: '%s'", value);
-			goto fail;
-		}
 		sm->ignore_default = sm->ignore = (git_submodule_ignore_t)val;
 	}
 	/* ignore other unknown submodule properties */
 
 	return 0;
-
-fail:
-	submodule_release(sm, 0);
-	git_buf_free(&name);
-	return -1;
 }
 
 static int submodule_load_from_wd_lite(
@@ -1117,10 +1119,9 @@ static git_config_file *open_gitmodules(
 		if (okay_to_create || git_path_isfile(path.ptr)) {
 			/* git_config_file__ondisk should only fail if OOM */
 			if (git_config_file__ondisk(&mods, path.ptr) < 0)
-				return NULL;
+				mods = NULL;
-
 			/* open should only fail here if the file is malformed */
-			if (git_config_file_open(mods) < 0) {
+			else if (git_config_file_open(mods) < 0) {
 				git_config_file_free(mods);
 				mods = NULL;
 			}
@@ -1135,6 +1136,8 @@ static git_config_file *open_gitmodules(
 		 */
 	}
 
+	git_buf_free(&path);
+
 	return mods;
 }
 

tests-clar/submodule/modify.c

@@ -253,4 +253,5 @@ void test_submodule_modify__edit_and_save(void)
 		(int)GIT_SUBMODULE_UPDATE_REBASE, (int)git_submodule_update(sm2));
 
 	git_repository_free(r2);
+	git__free(old_url);
 }

tests-clar/valgrind-supp-mac.txt

@@ -0,0 +1,82 @@
+{
+   libgit2-giterr-set-buffer
+   Memcheck:Leak
+   ...
+   fun:git__realloc
+   fun:git_buf_try_grow
+   fun:git_buf_grow
+   fun:git_buf_vprintf
+   fun:giterr_set
+}
+{
+   mac-setenv-leak-1
+   Memcheck:Leak
+   fun:malloc_zone_malloc
+   fun:__setenv
+   fun:setenv
+}
+{
+   mac-setenv-leak-2
+   Memcheck:Leak
+   fun:malloc_zone_malloc
+   fun:malloc_set_zone_name
+   ...
+   fun:init__zone0
+   fun:setenv
+}
+{
+   mac-dyld-initializer-leak
+   Memcheck:Leak
+   fun:malloc
+   ...
+   fun:dyld_register_image_state_change_handler
+   fun:_dyld_initializer
+}
+{
+   mac-tz-leak-1
+   Memcheck:Leak
+   ...
+   fun:token_table_add
+   fun:notify_register_check
+   fun:notify_register_tz
+}
+{
+   mac-tz-leak-2
+   Memcheck:Leak
+   fun:malloc
+   fun:tzload
+}
+{
+   mac-tz-leak-3
+   Memcheck:Leak
+   fun:malloc
+   fun:tzsetwall_basic
+}
+{
+   mac-tz-leak-4
+   Memcheck:Leak
+   fun:malloc
+   fun:gmtsub
+}
+{
+   mac-system-init-leak-1
+   Memcheck:Leak
+   ...
+   fun:_libxpc_initializer
+   fun:libSystem_initializer
+}
+{
+   mac-system-init-leak-2
+   Memcheck:Leak
+   ...
+   fun:__keymgr_initializer
+   fun:libSystem_initializer
+}
+{
+   mac-puts-leak
+   Memcheck:Leak
+   fun:malloc
+   fun:__smakebuf
+   ...
+   fun:puts
+}