Download raw body.
gotwebd: plug leak regarding repo_commit (plus bonus)
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;
> }
>
>
>
gotwebd: plug leak regarding repo_commit (plus bonus)