On 2022/08/15 08:53:05 +0200, Theo Buehler <t...@theobuehler.org> wrote:
> On Sun, Aug 14, 2022 at 11:15:16PM +0200, Omar Polo wrote:
> > I was looking for something different when I stumbled upon this.
> >
> > - in server_file_index `escapedpath' is leaked if evbuffer_add_printf
> > fails, similarly the directory entries in the loop. `escapeduri'
> > is also leaked if escape_html fails.
> >
> > - read_errdoc leaks a file descriptor if fstat fails or if the file
> > is empty.
> >
> > both very unlikely but worth fixing IMHO.
> >
> > For server_file_index I decided to just drop the `skip' variable and
> > just goto fail on errors. There shouldn't be any visible change from
> > it, except that we don't try to serve half page if we somehow exhaust
> > memory halfway through.
> >
> > ok?
>
> I have no opinion on whether keeping or dropping skip is better. There
> are a bunch of new problems with your diff, though.
Agree on all the points. I don't know how I haven't spotted them
re-reading the diff before sending it, apologize!
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.
Index: server_http.c
===================================================================
RCS file: /home/cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.150
diff -u -p -u -r1.150 server_http.c
--- server_http.c 2 Mar 2022 11:10:43 -0000 1.150
+++ server_http.c 15 Aug 2022 07:26:56 -0000
@@ -1777,13 +1777,16 @@ read_errdoc(const char *root, const char
free(path);
if (fstat(fd, &sb) < 0) {
log_warn("%s: stat", __func__);
+ close(fd);
return (NULL);
}
if ((ret = calloc(1, sb.st_size + 1)) == NULL)
fatal("calloc");
- if (sb.st_size == 0)
+ if (sb.st_size == 0) {
+ close(fd);
return (ret);
+ }
if (read(fd, ret, sb.st_size) != sb.st_size) {
log_warn("%s: read", __func__);
close(fd);
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 07:24:29 -0000
@@ -535,8 +535,10 @@ 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 fail;
+ }
free(escapedpath);
@@ -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')) {
