On 2022/09/06 12:16:15 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Sep 06, 2022 at 12:11:16PM +0200, Omar Polo wrote:
> > played again a bit with gotwebd under valgrind and found some other
> > leaks.  two are in gotwebd and the others in lib/.
> > 
> > some of the got_repo_get_* functions allocate a string that we forget
> > to free.  I did a round of checking and fixed the ones that were
> > (possibly) leaking memory.  Should we consider to rename some of this
> > functions to avoid the ambiguity?  (some of the got_repo_get_* returns
> > const char *, other don't.)
> 
> The return type (const char * vs. char *) should be enough to
> allow people to tell. Or just look into the implementation.
> 
> > @@ -923,6 +928,8 @@ done:
> >  	got_ref_list_free(&refs);
> >  	if (commit)
> >  		got_object_commit_close(commit);
> > +	if (tree)
> > +		got_object_tree_close(commit);
> 
> That doesn't look right (tree vs. commit)

yeah, sorry about that.  I manually copied the diff from the vm to my
server (where i'm building with CFLAGS='-O2 -pipe') and typoed that.

Here's a fixed diff with 100% less double frees

diff /home/op/w/got
commit - 93c74716961ac29893d89a1d807530c448a168b3
path + /home/op/w/got
blob - 1595b111128cb8eaca43db33c9b1eab5a5ff4291
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -923,6 +923,8 @@ done:
 	got_ref_list_free(&refs);
 	if (commit)
 		got_object_commit_close(commit);
+	if (tree)
+		got_object_tree_close(tree);
 	free(commit_id);
 	free(tree_id);
 	return error;
blob - b09e447cd160ea6b4ec21e61c49d40abcc7a3532
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -1629,21 +1629,27 @@ match_packed_object(struct got_object_id **unique_id,
     struct got_repository *repo, const char *id_str_prefix, int obj_type)
 {
 	const struct got_error *err = NULL;
+	char *objects_pack_dir = NULL;
 	struct got_object_id_queue matched_ids;
 	struct got_pathlist_entry *pe;
 	struct stat sb;
 
 	STAILQ_INIT(&matched_ids);
 
-	if (stat(got_repo_get_path_objects_pack(repo), &sb) == -1) {
-		if (errno != ENOENT)
-			return got_error_from_errno2("stat",
-			    got_repo_get_path_objects_pack(repo));
+	objects_pack_dir = got_repo_get_path_objects_pack(repo);
+	if (objects_pack_dir == NULL)
+		return got_error_from_errno("got_repo_get_path_objects_pack");
+
+	if (stat(objects_pack_dir, &sb) == -1) {
+		if (errno != ENOENT) {
+			err = got_error_from_errno2("stat", objects_pack_dir);
+			goto done;
+		}
 	} else if (sb.st_mtime != repo->pack_path_mtime) {
 		purge_packidx_paths(&repo->packidx_paths);
 		err = got_repo_list_packidx(&repo->packidx_paths, repo);
 		if (err)
-			return err;
+			goto done;
 	}
 
 	TAILQ_FOREACH(pe, &repo->packidx_paths, entry) {
@@ -1692,6 +1698,7 @@ match_packed_object(struct got_object_id **unique_id,
 		}
 	}
 done:
+	free(objects_pack_dir);
 	got_object_id_queue_free(&matched_ids);
 	if (err) {
 		free(*unique_id);
@@ -1790,21 +1797,25 @@ got_repo_match_object_id_prefix(struct got_object_id *
     const char *id_str_prefix, int obj_type, struct got_repository *repo)
 {
 	const struct got_error *err = NULL;
-	char *path_objects = got_repo_get_path_objects(repo);
-	char *object_dir = NULL;
+	char *path_objects = NULL, *object_dir = NULL;
 	size_t len;
 	int i;
 
 	*id = NULL;
 
+	path_objects = got_repo_get_path_objects(repo);
+
 	len = strlen(id_str_prefix);
-	if (len > SHA1_DIGEST_STRING_LENGTH - 1)
-		return got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR);
+	if (len > SHA1_DIGEST_STRING_LENGTH - 1) {
+		err = got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR);
+		goto done;
+	}
 
 	for (i = 0; i < len; i++) {
 		if (isxdigit((unsigned char)id_str_prefix[i]))
 			continue;
-		return got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR);
+		err = got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR);
+		goto done;
 	}
 
 	if (len >= 2) {
@@ -1840,6 +1851,7 @@ got_repo_match_object_id_prefix(struct got_object_id *
 		goto done;
 	}
 done:
+	free(path_objects);
 	free(object_dir);
 	if (err) {
 		free(*id);