Download raw body.
gotwebd: fix null-deref and ENOMEM question
On 2022/09/01 20:35:58 +0200, Omar Polo <op@omarpolo.com> wrote:
> On 2022/09/01 17:44:17 +0200, Omar Polo <op@omarpolo.com> wrote:
> > On 2022/08/25 17:41:46 +0200, Omar Polo <op@omarpolo.com> 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
and here's an improved version with a more usual resource handling.
I've moved repo_dir back to the function scope and free it only when
needed.
I'm still not 100% happy about the diff, see in particular the else
branch I had to add, but it fixes most of the leaks and we can work on
simplifying this later.
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
@@ -338,6 +338,7 @@ got_get_repo_commits(struct request *c, int limit)
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 +356,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,
@@ -441,8 +438,8 @@ got_get_repo_commits(struct request *c, int limit)
for (;;) {
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 +465,44 @@ 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");
+ 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);
+ repo_commit = NULL;
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);
+ /* repo_commit = NULL; */
- 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,10 +525,19 @@ 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);
+ repo_commit = NULL;
goto done;
+ } else {
+ /*
+ * We've queued the commit and decided
+ * to keep it; set it to NULL so we
+ * don't try to free it later or on
+ * error.
+ */
+ repo_commit = NULL;
}
}
got_ref_list_free(&refs);
@@ -542,15 +552,25 @@ got_get_repo_commits(struct request *c, int limit)
got_object_commit_close(commit);
commit = NULL;
}
+ if (repo_commit) {
+ gotweb_free_repo_commit(repo_commit);
+ repo_commit = NULL;
+ }
}
done:
- gotweb_free_repo_commit(repo_commit);
+ if (repo_commit)
+ 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;
gotwebd: fix null-deref and ENOMEM question