On Mon, Aug 15, 2022 at 09:38:51AM +0200, Omar Polo wrote:
> 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!
No worries. This is super convoluted and tricky code. Everyone who
tried to fix something in httpd learned this the hard way :)
This diff is better. Let's land server_http.c so it is out of the way,
ok for that.
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).
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().
> 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.
>
>
> 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')) {
