On Thu, Apr 14, 2022 at 10:28:21AM +0200, Stefan Sperling wrote:
> On Thu, Apr 14, 2022 at 08:59:49AM +0200, Omar Polo wrote:
> > Stefan Sperling <stsp@stsp.name> wrote:
> > > In any case, as a first step, we should check the return value.
> > > 
> > > ok?
> > 
> > I like the idea, and the patch looks fine, modulo a minor nit.  With a
> > bit more of context on got_object_idset_add:
> > 
> >  	new = malloc(sizeof(*new));
> >  	if (new == NULL)
> >  		return got_error_from_errno("malloc");
> >  
> >  	memcpy(&new->id, id, sizeof(new->id));
> >  	new->data = data;
> >  
> > -	RB_INSERT(got_object_idset_tree, &set->entries, new)
> > +	if (RB_INSERT(got_object_idset_tree, &set->entries, new) != NULL)
> > +		return got_error(GOT_ERR_OBJ_EXISTS);
> > 
> > shouldn't we free `new' here before returning the error?  At the moment
> > this is innocuos since we check that element isn't present before adding
> > it, but later this would turn into a leak.
> 
> Yes, indeed, should free on error. Thanks!

This is independent of your diff, but op made me look: callers of
got_object_idset_add() also need to be careful to free the data they
passed in on error. I think you want something like this as well
(apologies for the git diff):

diff --git lib/pack_create.c lib/pack_create.c
index b52856ef..4e9a9b95 100644
--- lib/pack_create.c
+++ lib/pack_create.c
@@ -867,11 +867,19 @@ add_object(int want_meta, struct got_object_idset *idset,
 		(*nfound)++;
 		err = report_progress(progress_cb, progress_arg, rl,
 		    *ncolored, *nfound, *ntrees, 0L, 0, 0, 0, 0);
-		if (err)
+		if (err) {
+			clear_meta(m);
+			free(m);
 			return err;
+		}
 	}
 
-	return got_object_idset_add(idset, id, m);
+	err = got_object_idset_add(idset, id, m);
+	if (err) {
+		clear_meta(m);
+		free(m);
+	}
+	return err;
 }
 
 static const struct got_error *
diff --git lib/reference.c lib/reference.c
index ea1f566d..8616b4e5 100644
--- lib/reference.c
+++ lib/reference.c
@@ -1569,8 +1569,10 @@ add_object_id_map_entry(struct got_object_idset *idset,
 
 		TAILQ_INIT(&ent->refs);
 		err = got_object_idset_add(idset, id, ent);
-		if (err)
+		if (err) {
+			free(ent);
 			return err;
+		}
 	}
 
 	err = got_reflist_entry_dup(&new, re);