From: Stefan Sperling Subject: Re: gotwebd: plug leak regarding repo_commit (plus bonus) To: Omar Polo Cc: gameoftrees@openbsd.org Date: Thu, 1 Sep 2022 11:50:54 +0200 On Wed, Aug 31, 2022 at 11:24:36PM +0200, Omar Polo wrote: > The `repo_tag' structure are not properly free'd. The tag_name and > tag_commit field are left leaking. How tag_commit is populated in > got_get_repo_tags needs some tweaking too. > > While here I'm bundling as a bonus ;-) some other minor tweaks spotted > while hunting for memleaks. There are a couple of `return error' that > leaks memory on error (unlikely), a tautological s[strlen(s)] == '\0' > dropped, a few duplicate checks remove (the t->next_id == NULL) and a > lone if (error) when it's always NULL too. In gotweb_free_transport > I'm taking the chance to drop an unneeded NULL check of t. > > ok? ok stsp; everything in here looks like an improvement :) > diff /home/op/w/got > commit - b765b7a889c2e1d41b6b0d21118c549fdc2b969e > path + /home/op/w/got > blob - 85594b289ee57c52802a8fa7745f70d2a9bb3e77 > file + gotwebd/got_operations.c > --- gotwebd/got_operations.c > +++ gotwebd/got_operations.c > @@ -227,8 +227,7 @@ got_get_repo_commit(struct request *c, struct repo_com > name += 6; > if (strncmp(name, "remotes/", 8) == 0) { > name += 8; > - s = strstr(name, "/" GOT_REF_HEAD); > - if (s != NULL && s[strlen(s)] == '\0') > + if (strstr(name, "/" GOT_REF_HEAD) != NULL) > continue; > } > error = got_ref_resolve(&ref_id, t->repo, re->ref); > @@ -351,12 +350,14 @@ got_get_repo_commits(struct request *c, int limit) > return got_error_from_errno("asprintf"); > > if (asprintf(&repo_path, "%s/%s", srv->repos_path, > - repo_dir->name) == -1) > - return got_error_from_errno("asprintf"); > + repo_dir->name) == -1) { > + error = got_error_from_errno("asprintf"); > + goto done; > + } > > error = got_init_repo_commit(&repo_commit); > if (error) > - return error; > + goto done; > > /* > * XXX: jumping directly to a commit id via > @@ -523,10 +524,6 @@ got_get_repo_commits(struct request *c, int limit) > got_object_commit_close(commit); > commit = NULL; > } > - if (t->next_id == NULL) { > - error = got_error_from_errno("strdup"); > - goto done; > - } > TAILQ_REMOVE(&t->repo_commits, new_repo_commit, > entry); > gotweb_free_repo_commit(new_repo_commit); > @@ -576,6 +573,7 @@ got_get_repo_tags(struct request *c, int limit) > struct got_tag_object *tag = NULL; > struct repo_tag *rt = NULL, *trt = NULL; > char *in_repo_path = NULL, *repo_path = NULL, *id_str = NULL; > + char *tag_commit = NULL, *tag_commit0 = NULL; > char *commit_msg = NULL, *commit_msg0 = NULL; > int chk_next = 0, chk_multi = 1, commit_found = 0, c_cnt = 0; > > @@ -585,9 +583,6 @@ got_get_repo_tags(struct request *c, int limit) > repo_dir->name) == -1) > return got_error_from_errno("asprintf"); > > - if (error) > - return error; > - > if (qs->commit == NULL && qs->action == TAGS) { > error = got_ref_open(&ref, repo, qs->headref, 0); > if (error) > @@ -724,33 +719,36 @@ got_get_repo_tags(struct request *c, int limit) > got_object_commit_close(commit); > commit = NULL; > } > - if (t->next_id == NULL) { > - error = got_error_from_errno("strdup"); > - goto err; > - } > TAILQ_REMOVE(&t->repo_tags, new_repo_tag, entry); > gotweb_free_repo_tag(new_repo_tag); > goto done; > } > > if (commit) { > - error = got_object_commit_get_logmsg(&new_repo_tag-> > - tag_commit, commit); > + error = got_object_commit_get_logmsg(&tag_commit0, > + commit); > if (error) > - goto done; > + goto err; > got_object_commit_close(commit); > commit = NULL; > } else { > - new_repo_tag->tag_commit = > - strdup(got_object_tag_get_message(tag)); > - if (new_repo_tag->tag_commit == NULL) { > + tag_commit0 = strdup(got_object_tag_get_message(tag)); > + if (tag_commit0 == NULL) { > error = got_error_from_errno("strdup"); > - goto done; > + goto err; > } > } > > - while (*new_repo_tag->tag_commit == '\n') > - new_repo_tag->tag_commit++; > + tag_commit = tag_commit0; > + while (*tag_commit == '\n') > + tag_commit++; > + new_repo_tag->tag_commit = strdup(tag_commit); > + if (new_repo_tag->tag_commit == NULL) { > + error = got_error_from_errno("strdup"); > + free(tag_commit0); > + goto err; > + } > + free(tag_commit0); > > if (qs->action != SUMMARY && qs->action != TAGS) { > commit_msg = commit_msg0; > blob - aa8a0d92f2c47069f782a2be91a69ccd329c0c12 > file + gotwebd/gotweb.c > --- gotwebd/gotweb.c > +++ gotwebd/gotweb.c > @@ -540,8 +540,10 @@ void > gotweb_free_repo_tag(struct repo_tag *rt) > { > if (rt != NULL) { > - free(rt->commit_msg); > free(rt->commit_id); > + free(rt->tag_name); > + free(rt->tag_commit); > + free(rt->commit_msg); > free(rt->tagger); > } > free(rt); > @@ -608,10 +610,8 @@ gotweb_free_transport(struct transport *t) > } > gotweb_free_repo_dir(t->repo_dir); > gotweb_free_querystring(t->qs); > - if (t != NULL) { > - free(t->next_id); > - free(t->prev_id); > - } > + free(t->next_id); > + free(t->prev_id); > free(t); > } > > @@ -2026,9 +2026,8 @@ gotweb_get_repo_description(char **description, struct > > f = fopen(d_file, "r"); > if (f == NULL) { > - if (errno == ENOENT || errno == EACCES) > - return NULL; > - error = got_error_from_errno2("fopen", d_file); > + if (errno != ENOENT && errno != EACCES) > + error = got_error_from_errno2("fopen", d_file); > goto done; > } > > @@ -2045,7 +2044,7 @@ gotweb_get_repo_description(char **description, struct > if (len == 0) { > *description = strdup(""); > if (*description == NULL) > - return got_error_from_errno("strdup"); > + error = got_error_from_errno("strdup"); > goto done; > } > > >