From: Omar Polo <op@omarpolo.com>
Subject: Re: gotd protected references
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 04 Apr 2023 21:12:39 +0200

On 2023/04/04 20:17:24 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Apr 04, 2023 at 07:56:29PM +0200, Omar Polo wrote:
> > On 2023/04/04 13:31:07 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > > +	error = got_pathlist_insert(NULL, &repo->protected_branches,
> > > +	    refname, NULL);
> > > +	if (error) {
> > > +		yyerror("got_pathlist_insert: %s", error->msg);
> > 
> > nit: we could free(refname) here.
> 
> No. got_pathlist_insert() does not copy the string. It adds the
> naked pointer to the list.  This is by design because some callers
> add const char * variables which cannot be freed because they point
> to strings that are were not allocated with malloc.

Yeah, but my point was that in the error path the insertion was not
done and the memory leaked.  We're also not handling the case of a
duplicate entry.  Since it's easier I'm considering it a syntax error
too.

If we agree to disallow duplicates it could be interesting to catch
redundant namespaces too, like

	protect branch namespace foo/bar/baz/
	protect branch namespace foo/


diff /home/op/w/gotacl
commit - 9afa3de221045d529287cc3fa75fdc2915aed5c1
path + /home/op/w/gotacl
blob - 3b6daa1a2a93121b0ee884ddd8cd1ceb5e85fa9b
file + gotd/parse.y
--- gotd/parse.y
+++ gotd/parse.y
@@ -918,6 +918,7 @@ conf_protect_ref_namespace(struct got_pathlist_head *r
 conf_protect_ref_namespace(struct got_pathlist_head *refs, char *namespace)
 {
 	const struct got_error *error;
+	struct got_pathlist_entry *new;
 	char *s;
 
 	got_path_strip_trailing_slashes(namespace);
@@ -928,9 +929,13 @@ conf_protect_ref_namespace(struct got_pathlist_head *r
 		return -1;
 	}
 
-	error = got_pathlist_insert(NULL, refs, s, NULL);
-	if (error) {
-		yyerror("got_pathlist_insert: %s", error->msg);
+	error = got_pathlist_insert(&new, refs, s, NULL);
+	if (error || new == NULL) {
+		free(s);
+		if (error)
+			yyerror("got_pathlist_insert: %s", error->msg);
+		else
+			yyerror("duplicate protect namespace %s", namespace);
 		return -1;
 	}
 
@@ -955,6 +960,7 @@ conf_protect_branch(struct gotd_repo *repo, char *bran
 conf_protect_branch(struct gotd_repo *repo, char *branchname)
 {
 	const struct got_error *error;
+	struct got_pathlist_entry *new;
 	char *refname;
 
 	if (strncmp(branchname, "refs/heads/", 11) != 0) {
@@ -975,10 +981,14 @@ conf_protect_branch(struct gotd_repo *repo, char *bran
 		return -1;
 	}
 
-	error = got_pathlist_insert(NULL, &repo->protected_branches,
+	error = got_pathlist_insert(&new, &repo->protected_branches,
 	    refname, NULL);
-	if (error) {
-		yyerror("got_pathlist_insert: %s", error->msg);
+	if (error || new == NULL) {
+		free(refname);
+		if (error)
+			yyerror("got_pathlist_insert: %s", error->msg);
+		else
+			yyerror("duplicate protect branch %s", branchname);
 		return -1;
 	}