From: Omar Polo Subject: Re: gotwebd: fix null-deref and ENOMEM question To: Omar Polo Cc: Tracey Emery , gameoftrees@openbsd.org Date: Thu, 01 Sep 2022 20:35:58 +0200 On 2022/09/01 17:44:17 +0200, Omar Polo wrote: > On 2022/08/25 17:41:46 +0200, Omar Polo wrote: > > I'm still seeing gotwebd increasing its memory usage over time so I'm > > still searching for leaks. > > > > [...] > > > > this still doesn't explain why gotwebd reached 600M of RES in hours, > > so there's probably more. > > one big leak was in gotweb_render_index where we fail to free the data > allocated by scandir(3) > > with this is, gotwebd memory consumption didn't raised after a few > thousands requests (I'm running ftp in a loop against it)! \o/ I phrased this poorly; i'm going page by page and the listing page is now memleaks free (well, for the most part at least :D) diff below tries to fix most of the leaks in the repo summary page. definitely needs more eyeballs because i'm not sure anymore what I'm doing. There are a simple fix in it, like forgetting to free some strings or close the commits in some code paths, but the big leak is `repo_commit' that get filled each time with various strings, and each time we leak the data of the previous run. To fix it I'm trying to avoid doing a double got_get_repo_commit in the loop and doing it just once. From a quick look seems to even work! :O diff /home/op/w/got commit - 2db401bd3a14512e3a1f1cbe686fff37b2c56764 path + /home/op/w/got blob - ac3b54fcdf929d17f3aea822d4162937114cf8a7 file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -332,12 +332,12 @@ got_get_repo_commits(struct request *c, int limit) struct got_commit_object *commit = NULL; struct got_reflist_head refs; struct got_reference *ref; - struct repo_commit *repo_commit = NULL; struct server *srv = c->srv; struct transport *t = c->t; struct got_repository *repo = t->repo; struct querystring *qs = t->qs; struct repo_dir *repo_dir = t->repo_dir; + char *commit_id = NULL; char *in_repo_path = NULL, *repo_path = NULL, *file_path = NULL; int chk_next = 0, chk_multi = 0, commit_found = 0; int obj_type, limit_chk = 0; @@ -355,10 +355,6 @@ got_get_repo_commits(struct request *c, int limit) goto done; } - error = got_init_repo_commit(&repo_commit); - if (error) - goto done; - /* * XXX: jumping directly to a commit id via * got_repo_match_object_id_prefix significantly improves performance, @@ -440,9 +436,11 @@ got_get_repo_commits(struct request *c, int limit) goto done; for (;;) { + struct repo_commit *repo_commit = NULL; + if (limit_chk == ((limit * qs->page) - (limit - 1)) && - commit_found == 0 && repo_commit->commit_id != NULL) { - t->prev_id = strdup(repo_commit->commit_id); + commit_found == 0 && commit_id != NULL) { + t->prev_id = strdup(commit_id); if (t->prev_id == NULL) { error = got_error_from_errno("strdup"); goto done; @@ -468,40 +466,43 @@ got_get_repo_commits(struct request *c, int limit) if (error) goto done; + error = got_init_repo_commit(&repo_commit); + if (error) + goto done; + error = got_get_repo_commit(c, repo_commit, commit, - &refs, id); + &refs, id); if (error) goto done; + free(commit_id); + commit_id = strdup(repo_commit->commit_id); + if (commit_id == NULL) { + error = got_error_from_errno("strdup"); + gotweb_free_repo_commit(repo_commit); + goto done; + } + free(id); + id = NULL; + if (qs->commit != NULL && commit_found == 0 && limit != 1) { - if (strcmp(qs->commit, repo_commit->commit_id) == 0) + if (strcmp(qs->commit, commit_id) == 0) commit_found = 1; else if (qs->file != NULL && strlen(qs->file) > 0 && qs->page == 0) commit_found = 1; else { limit_chk++; - free(id); - id = NULL; + got_object_commit_close(commit); + commit = NULL; + got_ref_list_free(&refs); + gotweb_free_repo_commit(repo_commit); continue; } } - struct repo_commit *new_repo_commit = NULL; - error = got_init_repo_commit(&new_repo_commit); - if (error) - goto done; + TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry); - TAILQ_INSERT_TAIL(&t->repo_commits, new_repo_commit, entry); - - error = got_get_repo_commit(c, new_repo_commit, commit, - &refs, id); - if (error) - goto done; - - free(id); - id = NULL; - if (limit == 1 && chk_multi == 0 && srv->max_commits_display != 1) commit_found = 1; @@ -515,7 +516,7 @@ got_get_repo_commits(struct request *c, int limit) */ if (chk_next && (qs->action == BRIEFS || qs->action == COMMITS || qs->action == SUMMARY)) { - t->next_id = strdup(new_repo_commit->commit_id); + t->next_id = strdup(commit_id); if (t->next_id == NULL) { error = got_error_from_errno("strdup"); goto done; @@ -524,9 +525,9 @@ got_get_repo_commits(struct request *c, int limit) got_object_commit_close(commit); commit = NULL; } - TAILQ_REMOVE(&t->repo_commits, new_repo_commit, + TAILQ_REMOVE(&t->repo_commits, repo_commit, entry); - gotweb_free_repo_commit(new_repo_commit); + gotweb_free_repo_commit(repo_commit); goto done; } } @@ -544,13 +545,17 @@ got_get_repo_commits(struct request *c, int limit) } } done: - gotweb_free_repo_commit(repo_commit); if (commit) got_object_commit_close(commit); if (graph) got_commit_graph_close(graph); + if (commit) + got_object_commit_close(commit); got_ref_list_free(&refs); + free(commit_id); free(file_path); + free(in_repo_path); + free(file_path); free(repo_path); free(id); return error;