On 2022/08/15 10:13:07 +0200, Theo Buehler <t...@theobuehler.org> wrote: > I think you should move the scandir() call to right before the for loop. > This way the two goto fail before the for loop don't leak the namelist > (one of these leaks was already there, the other one is added in your > diff).
right, with the skip=1 -> goto change I'm now leaking the namelist... I like the idea of moving scandir later! > I'm a bit unclear on the distinction between goto fail and goto abort. > I believe all goto fail should actually be goto abort until > server_response_http(). Agreed. Even if some operation failed we can still generate an error page; only after server_response_http things get trickier (headers are set, logging is done, ...) and just shutting down the reply is easier. (it also becomes a different problem, it's no more "failed to generate the page" but rather to send it.) I'm changing the only offending `goto fail' to go to abort too since I'm here. > > I tried to do too much in a single diff. Instead of a refactoring + > > memleak, here's a diff that only covers the leaks. I still don't like > > how it still tries to server_response_http when skip = 1, but that can > > be address later. > > It does not try to do server_response_http() in that situation. There is > > if (skip || ...) > goto abort; > > after the for loop. TIL that I can't read! lesson learned, will never try again to send a diff later the night or earlier the morning without having had coffee before :) Thanks! Index: server_file.c =================================================================== RCS file: /home/cvs/src/usr.sbin/httpd/server_file.c,v retrieving revision 1.74 diff -u -p -u -r1.74 server_file.c --- server_file.c 4 Mar 2022 01:46:07 -0000 1.74 +++ server_file.c 15 Aug 2022 08:34:54 -0000 @@ -507,14 +507,8 @@ server_file_index(struct httpd *env, str if ((evb = evbuffer_new()) == NULL) goto abort; - if ((namesize = scandir(path, &namelist, NULL, alphasort)) == -1) - goto abort; - - /* Indicate failure but continue going through the list */ - skip = 0; - if ((escapedpath = escape_html(desc->http_path)) == NULL) - goto fail; + goto abort; /* A CSS stylesheet allows minimal customization by the user */ style = "body { background-color: white; color: black; font-family: " @@ -535,11 +529,19 @@ server_file_index(struct httpd *env, str "<body>\n" "<h1>Index of %s</h1>\n" "<hr>\n<pre>\n", - escapedpath, style, escapedpath) == -1) - skip = 1; + escapedpath, style, escapedpath) == -1) { + free(escapedpath); + goto abort; + } free(escapedpath); + if ((namesize = scandir(path, &namelist, NULL, alphasort)) == -1) + goto abort; + + /* Indicate failure but continue going through the list */ + skip = 0; + for (i = 0; i < namesize; i++) { struct stat subst; @@ -556,10 +558,17 @@ server_file_index(struct httpd *env, str strftime(tmstr, sizeof(tmstr), "%d-%h-%Y %R", &tm); namewidth = 51 - strlen(dp->d_name); - if ((escapeduri = url_encode(dp->d_name)) == NULL) - goto fail; - if ((escapedhtml = escape_html(dp->d_name)) == NULL) - goto fail; + if ((escapeduri = url_encode(dp->d_name)) == NULL) { + skip = 1; + free(dp); + continue; + } + if ((escapedhtml = escape_html(dp->d_name)) == NULL) { + skip = 1; + free(escapeduri); + free(dp); + continue; + } if (dp->d_name[0] == '.' && !(dp->d_name[1] == '.' && dp->d_name[2] == '\0')) {