On 2022/08/15 10:13:07 +0200, Theo Buehler <[email protected]> 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')) {
