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.
>
>
> Index: server_file.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/httpd/server_file.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 server_file.c
> --- server_file.c 4 Mar 2022 01:46:07 -0000 1.74
> +++ server_file.c 14 Aug 2022 21:09:25 -0000
> @@ -474,7 +474,7 @@ server_file_index(struct httpd *env, str
> struct http_descriptor *desc = clt->clt_descreq;
> struct server_config *srv_conf = clt->clt_srv_conf;
> struct dirent **namelist, *dp;
namelist should now be initialized to NULL since you can goto abort
before it is initialized.
> - int namesize, i, ret, fd = -1, namewidth, skip;
> + int namesize = 0, i = 0, ret, fd = -1, namewidth;
> int code = 500;
> struct evbuffer *evb = NULL;
> struct media_type *media;
> @@ -510,9 +510,6 @@ server_file_index(struct httpd *env, str
> 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;
>
> @@ -535,8 +532,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);
>
> @@ -545,8 +544,7 @@ server_file_index(struct httpd *env, str
>
> dp = namelist[i];
>
> - if (skip ||
> - fstatat(fd, dp->d_name, &subst, 0) == -1) {
> + if (fstatat(fd, dp->d_name, &subst, 0) == -1) {
> free(dp);
> continue;
> }
> @@ -558,8 +556,10 @@ server_file_index(struct httpd *env, str
>
> if ((escapeduri = url_encode(dp->d_name)) == NULL)
> goto fail;
> - if ((escapedhtml = escape_html(dp->d_name)) == NULL)
> + if ((escapedhtml = escape_html(dp->d_name)) == NULL) {
> + free(escapeduri);
> goto fail;
> + }
>
> if (dp->d_name[0] == '.' &&
> !(dp->d_name[1] == '.' && dp->d_name[2] == '\0')) {
> @@ -571,7 +571,7 @@ server_file_index(struct httpd *env, str
> strchr(escapeduri, ':') != NULL ? "./" : "",
> escapeduri, escapedhtml,
> MAXIMUM(namewidth, 0), " ", tmstr, "-") == -1)
> - skip = 1;
> + goto fail;
this goto now leaks escapeduri and escapedhtml
> } else if (S_ISREG(subst.st_mode)) {
> if (evbuffer_add_printf(evb,
> "<a href=\"%s%s\">%s</a>%*s%s%20llu\n",
> @@ -579,16 +579,16 @@ server_file_index(struct httpd *env, str
> escapeduri, escapedhtml,
> MAXIMUM(namewidth, 0), " ",
> tmstr, subst.st_size) == -1)
> - skip = 1;
> + goto fail;
ditto
> }
> free(escapeduri);
> free(escapedhtml);
If I'd want to drop skip, I'd probably add
escapeduri = NULL;
escapedhtml = NULL;
here and free them also in the fail path. Of course you then mustn't
free escapeduri if escape_html() fails.
> free(dp);
> }
> free(namelist);
> + namelist = NULL;
>
> - if (skip ||
> - evbuffer_add_printf(evb,
> + if (evbuffer_add_printf(evb,
> "</pre>\n<hr>\n</body>\n</html>\n") == -1)
> goto abort;
>
> @@ -629,6 +629,9 @@ server_file_index(struct httpd *env, str
> bufferevent_free(clt->clt_bev);
> clt->clt_bev = NULL;
> abort:
> + for (; i < namesize; i++)
> + free(namelist[i]);
> + free(namelist);
> if (fd != -1)
> close(fd);
> if (evb != NULL)
> Index: server_http.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.150
> diff -u -p -r1.150 server_http.c
> --- server_http.c 2 Mar 2022 11:10:43 -0000 1.150
> +++ server_http.c 14 Aug 2022 18:42:33 -0000
> @@ -1777,14 +1777,13 @@ 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)
No strong opinion, but I'd prefer adding a close here. httpd code is
already tricky enough to read.
> - return (ret);
> - if (read(fd, ret, sb.st_size) != sb.st_size) {
> + if (sb.st_size != 0 && read(fd, ret, sb.st_size) != sb.st_size) {
> log_warn("%s: read", __func__);
> close(fd);
> free(ret);
>
