Re: httpd: patch for portability asprintf use

[Joerg Jung]

On Fri, May 06, 2016 at 06:48:38PM +0200, Reyk Floeter wrote:
> 
> > On 06.05.2016, at 18:36, Theo de Raadt  wrote:
> > 
> >> If OpenBSD's behavior of asprintf is non-standard and everyone else is
> >> doing it differently, we would probably have to apply the patch. But this
> >> would also affect many other places in the tree were we rely on our
> >> asprintf semantics.
> > 
> > Actually, we have fixed all usage cases in our tree to be portable.
> > 
> > I have wondered in the past whether we should set the pointer to (void
> > *)-1 instead of NULL, because this NULL return is a trap.
> > 
> 
> 
> I think enforcing it this way would make much sense.
> 
> I'm a candidate for such traps as I only develop C code on/for OpenBSD.
> 
> OK, this makes Hiltjo's diff a valid addition.

yes agreed, please commit it. ok jung@
 
> Reyk
> 

Re: httpd: patch for portability asprintf use

[Todd C. Miller]

On Fri, 06 May 2016 18:53:28 +0200, Hiltjo Posthuma wrote:

> It is not an issue, but I thought it was not intended because the
> asprintf(3) documentation said:
> 
> "The value of ret in this situation is
> implementation-dependent (on OpenBSD, ret will be set to the null
> pointer, >but this behavior should not be relied upon<)."

Some time ago there was talk of changing the documented value of
ret on error in glibc, but it doesn't seem to have happened.

In light of that, it is probably best to explicitly NULL out the
value as per your patch.

 - todd

Re: httpd: patch for portability asprintf use

[Ted Unangst]

Theo de Raadt wrote:
> > If OpenBSD's behavior of asprintf is non-standard and everyone else is
> > doing it differently, we would probably have to apply the patch. But this
> > would also affect many other places in the tree were we rely on our
> > asprintf semantics.
> 
> Actually, we have fixed all usage cases in our tree to be portable.
> 
> I have wondered in the past whether we should set the pointer to (void
> *)-1 instead of NULL, because this NULL return is a trap.

interesting idea..

Re: httpd: patch for portability asprintf use

[Theo de Raadt]

> Theo de Raadt wrote:
> > > If OpenBSD's behavior of asprintf is non-standard and everyone else is
> > > doing it differently, we would probably have to apply the patch. But this
> > > would also affect many other places in the tree were we rely on our
> > > asprintf semantics.
> > 
> > Actually, we have fixed all usage cases in our tree to be portable.
> > 
> > I have wondered in the past whether we should set the pointer to (void
> > *)-1 instead of NULL, because this NULL return is a trap.
> 
> interesting idea..

But then we should also gaurantee the last page of memory cannot
be mapped by userland, same treatment as the NULL page.

(Not all architectures behave like i386)

Re: httpd: patch for portability asprintf use

[Hiltjo Posthuma]

On Fri, May 6, 2016 at 6:47 PM, Todd C. Miller
 wrote:
> On Fri, 06 May 2016 17:56:16 +0200, Hiltjo Posthuma wrote:
>
>> Any OK's for this?
>>
>> Please also see my mail with subject "httpd: fix/style: unbalanced
>> va_start and va_end macros" (don't want to spam the mailinglist :)).
>
> I didn't change those before because body and hstsheader are
> initialized to NULL.  All known asprintf() implementations either
> leave the ret pointer unchanged on error or set it to NULL.
>
> So this would only be a problem on systems where asprintf() modifies
> ret on error and sets it to a non-NULL value.  Is that really an
> issue?
>

It is not an issue, but I thought it was not intended because the
asprintf(3) documentation said:

"The value of ret in this situation is
implementation-dependent (on OpenBSD, ret will be set to the null
pointer, >but this behavior should not be relied upon<)."

Kind regards,
Hiltjo

Re: httpd: patch for portability asprintf use

[Reyk Floeter]

> On 06.05.2016, at 18:36, Theo de Raadt  wrote:
> 
>> If OpenBSD's behavior of asprintf is non-standard and everyone else is
>> doing it differently, we would probably have to apply the patch. But this
>> would also affect many other places in the tree were we rely on our
>> asprintf semantics.
> 
> Actually, we have fixed all usage cases in our tree to be portable.
> 
> I have wondered in the past whether we should set the pointer to (void
> *)-1 instead of NULL, because this NULL return is a trap.
> 


I think enforcing it this way would make much sense.

I'm a candidate for such traps as I only develop C code on/for OpenBSD.

OK, this makes Hiltjo's diff a valid addition.

Reyk

Re: httpd: patch for portability asprintf use

[Todd C. Miller]

On Fri, 06 May 2016 17:56:16 +0200, Hiltjo Posthuma wrote:

> Any OK's for this?
> 
> Please also see my mail with subject "httpd: fix/style: unbalanced
> va_start and va_end macros" (don't want to spam the mailinglist :)).

I didn't change those before because body and hstsheader are
initialized to NULL.  All known asprintf() implementations either
leave the ret pointer unchanged on error or set it to NULL.

So this would only be a problem on systems where asprintf() modifies
ret on error and sets it to a non-NULL value.  Is that really an
issue?

 - todd

Re: httpd: patch for portability asprintf use

[Theo de Raadt]

> If OpenBSD's behavior of asprintf is non-standard and everyone else is
> doing it differently, we would probably have to apply the patch. But this
> would also affect many other places in the tree were we rely on our
> asprintf semantics.

Actually, we have fixed all usage cases in our tree to be portable.

I have wondered in the past whether we should set the pointer to (void
*)-1 instead of NULL, because this NULL return is a trap.

Re: httpd: patch for portability asprintf use

[Reyk Floeter]

> On 06.05.2016, at 17:56, Hiltjo Posthuma  wrote:
> 
> On Wed, Apr 27, 2016 at 2:41 PM, Hiltjo Posthuma  > wrote:
>> Hi,
>> 
>> The following patch for httpd makes sure the value of the asprintf buffer is
>> zeroed on error and not relied upon, so at the 'done' label free(body) and
>> free(hstsheader) is safe.
>> 
>> from asprintf(3):
>> 
>>"The asprintf() and vasprintf() functions return the number of 
>> characters
>>that were output to the newly allocated string (excluding the '\0').
>>A pointer to the newly allocated string is returned in ret; it
>>should be passed to free(3) to release the allocated storage when it 
>> is
>>no longer needed.  If sufficient space cannot be allocated, these
>>functions will return -1. >>The value of ret in this situation is
>>implementation-dependent (on OpenBSD, ret will be set to the null
>>pointer, but this behavior should not be relied upon)."<<
>> 
>> 
>> Index: server_http.c
>> ===
>> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
>> retrieving revision 1.106
>> diff -u -p -r1.106 server_http.c
>> --- server_http.c   8 Mar 2016 09:33:15 -   1.106
>> +++ server_http.c   27 Apr 2016 12:01:00 -
>> @@ -826,8 +826,10 @@ server_abort_http(struct client *clt, un
>>"\n%s\n"
>>"\n"
>>"\n",
>> -   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1)
>> +   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1) {
>> +   body = NULL;
>>goto done;
>> +   }
>> 
>>if (srv_conf->flags & SRVFLAG_SERVER_HSTS) {
>>if (asprintf(, "Strict-Transport-Security: "
>> @@ -835,8 +837,10 @@ server_abort_http(struct client *clt, un
>>srv_conf->hsts_flags & HSTSFLAG_SUBDOMAINS ?
>>"; includeSubDomains" : "",
>>srv_conf->hsts_flags & HSTSFLAG_PRELOAD ?
>> -   "; preload" : "") == -1)
>> +   "; preload" : "") == -1) {
>> +   hstsheader = NULL;
>>goto done;
>> +   }
>>}
>> 
>>/* Add basic HTTP headers */
>> 
>> --
> 
> Any OK's for this?
> 
> Please also see my mail with subject "httpd: fix/style: unbalanced
> va_start and va_end macros" (don't want to spam the mailinglist :)).

I don't know.

It doesn't affect OpenBSD and I don't like portability code in tree.
Isn't it better to create a portable version of httpd instead?

If OpenBSD's behavior of asprintf is non-standard and everyone else is
doing it differently, we would probably have to apply the patch. But this
would also affect many other places in the tree were we rely on our
asprintf semantics.

Reyk

Re: httpd: patch for portability asprintf use

[Hiltjo Posthuma]

On Wed, Apr 27, 2016 at 2:41 PM, Hiltjo Posthuma  wrote:
> Hi,
>
> The following patch for httpd makes sure the value of the asprintf buffer is
> zeroed on error and not relied upon, so at the 'done' label free(body) and
> free(hstsheader) is safe.
>
> from asprintf(3):
>
> "The asprintf() and vasprintf() functions return the number of 
> characters
> that were output to the newly allocated string (excluding the '\0').
> A pointer to the newly allocated string is returned in ret; it
> should be passed to free(3) to release the allocated storage when it 
> is
> no longer needed.  If sufficient space cannot be allocated, these
> functions will return -1. >>The value of ret in this situation is
> implementation-dependent (on OpenBSD, ret will be set to the null
> pointer, but this behavior should not be relied upon)."<<
>
>
> Index: server_http.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 server_http.c
> --- server_http.c   8 Mar 2016 09:33:15 -   1.106
> +++ server_http.c   27 Apr 2016 12:01:00 -
> @@ -826,8 +826,10 @@ server_abort_http(struct client *clt, un
> "\n%s\n"
> "\n"
> "\n",
> -   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1)
> +   code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1) {
> +   body = NULL;
> goto done;
> +   }
>
> if (srv_conf->flags & SRVFLAG_SERVER_HSTS) {
> if (asprintf(, "Strict-Transport-Security: "
> @@ -835,8 +837,10 @@ server_abort_http(struct client *clt, un
> srv_conf->hsts_flags & HSTSFLAG_SUBDOMAINS ?
> "; includeSubDomains" : "",
> srv_conf->hsts_flags & HSTSFLAG_PRELOAD ?
> -   "; preload" : "") == -1)
> +   "; preload" : "") == -1) {
> +   hstsheader = NULL;
> goto done;
> +   }
> }
>
> /* Add basic HTTP headers */
>
> --

Any OK's for this?

Please also see my mail with subject "httpd: fix/style: unbalanced
va_start and va_end macros" (don't want to spam the mailinglist :)).

> Kind regards,
> Hiltjo

Re: httpd patch

[Peter J. Philipp]

On Fri, Jan 15, 2016 at 08:36:05PM +, Peter J. Philipp wrote:
> Hello,
> 
> I had nothing better to do tonight after work so I read a little in httpd.
> I have come up with a patch for i386 and any architecture where off_t != 
> size_t.
> 
> So on i386 there is this:
> 
> uranus$ ./sizetest
> off_t = 8
> size_t = 4
> 
> and I have these files in a directory:
> 
> uranus$ ls -lhi
> total 12672
> 364207 -rw-r--r--  3 root  daemon   4.9G Jan 15 21:06 output.txt
> 364207 -rw-r--r--  3 root  daemon   4.9G Jan 15 21:06 output2.txt
> 364207 -rw-r--r--  3 root  daemon   4.9G Jan 15 21:06 output3.txt
> 
> A download (cancelled, but it doesn't matter) of the httpd without my patch
> looks like so:
> 
> default 192.168.1.127 - - [15/Jan/2016:21:11:55 +0100] "GET 
> /public/output2.txt HTTP/1.1" 200 948961280 "http://192.168.1.1/public/; 
> "Mozilla/5.0 (X11; OpenBSD amd64) AppleWebKit/537.36 (KHTML, like Gecko) 
> Chrome/44.0.2403.157 Safari/537.36"
> 
> A download (cancelled again) of the httpd with my patch looks like so:
> 
> default 192.168.1.127 - - [15/Jan/2016:21:18:07 +0100] "GET 
> /public/output3.txt HTTP/1.1" 200 5243928576 "http://192.168.1.1/public/; 
> "Mozilla/5.0 (X11; OpenBSD amd64) AppleWebKit/537.36 (KHTML, like Gecko) 
> Chrome/44.0.2403.157 Safari/537.36"
> 
> It looks accurate in this case.  Checking again with ls:
> 
> uranus$ ls -li output3.txt  
> 364207 -rw-r--r--  3 root  daemon  5243928576 Jan 15 21:06 output3.txt
> 
> Absolutely.
> 
> patch follows:
> 
> Cheers,
> -peter
> 
> ? httpd.patch
> Index: httpd.h
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
> retrieving revision 1.96
> diff -u -p -u -r1.96 httpd.h
> --- httpd.h   3 Aug 2015 11:45:17 -   1.96
> +++ httpd.h   15 Jan 2016 20:19:12 -
> @@ -602,7 +602,7 @@ const char *
>server_http_host(struct sockaddr_storage *, char *, size_t);
>  char *server_http_parsehost(char *, char *, size_t, int *);
>  ssize_t   server_http_time(time_t, char *, size_t);
> -int   server_log_http(struct client *, u_int, size_t);
> +int   server_log_http(struct client *, u_int, off_t);
>  
>  /* server_file.c */
>  int   server_file(struct httpd *, struct client *);
> Index: server_http.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.96
> diff -u -p -u -r1.96 server_http.c
> --- server_http.c 31 Jul 2015 00:10:51 -  1.96
> +++ server_http.c 15 Jan 2016 20:19:12 -
> @@ -1450,7 +1450,7 @@ server_httperror_cmp(const void *a, cons
>  }
>  
>  int
> -server_log_http(struct client *clt, u_int code, size_t len)
> +server_log_http(struct client *clt, u_int code, off_t len)
>  {
>   static char  tstamp[64];
>   static char  ip[INET6_ADDRSTRLEN];
> @@ -1511,7 +1511,7 @@ server_log_http(struct client *clt, u_in
>   goto done;
>  
>   ret = evbuffer_add_printf(clt->clt_log,
> - "%s %s - %s [%s] \"%s %s%s%s%s%s\" %03d %zu\n",
> + "%s %s - %s [%s] \"%s %s%s%s%s%s\" %03d %qu\n",
>   srv_conf->name, ip, clt->clt_remote_user == NULL ? "-" :
>   user, tstamp,
>   server_httpmethod_byid(desc->http_method),
> @@ -1559,7 +1559,7 @@ server_log_http(struct client *clt, u_in
>  
>   ret = evbuffer_add_printf(clt->clt_log,
>   "%s %s - %s [%s] \"%s %s%s%s%s%s\""
> - " %03d %zu \"%s\" \"%s\"\n",
> + " %03d %qu \"%s\" \"%s\"\n",
>   srv_conf->name, ip, clt->clt_remote_user == NULL ? "-" :
>   user, tstamp,
>   server_httpmethod_byid(desc->http_method),


Hello again,

I couldn't sleep because for some reason my head was spinning around this 
code.  In sleep I reviewed what I remembered of this code and noticed two
things.

1.  My patch was against 5.8 not -current, so it needed special hand fixing.

2.  The "Range" code required the same attention as my original attention.

I'm gonna need someone to look over my changes closely that I'm gonna put
forward next, because -current httpd doesn't compile on my system due to
some changes in SSL, and I don't want to fully go -current just yet on this
box (my only i386 box).  The following has a change in content_length from
size_t to off_t in function server_partial_file_request() because it does this:

content_length = range->end - range->start + 1;

and range->end and range->start are both off_t.

Here then the patches against -current (need review and testing):


? httpd.patch
Index: httpd.h
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.102
diff -u -p -u -r1.102 httpd.h
--- httpd.h 2 Dec 2015 15:13:00 -   1.102
+++ httpd.h 16 Jan 2016 04:22:50 -
@@ -597,7 

Re: httpd patch

[Peter J. Philipp]

On Sat, Jan 16, 2016 at 04:35:16AM +, Peter J. Philipp wrote:
> Hello again,
> 
> I couldn't sleep because for some reason my head was spinning around this 
> code.  In sleep I reviewed what I remembered of this code and noticed two
> things.
> 
> 1.  My patch was against 5.8 not -current, so it needed special hand fixing.
> 
> 2.  The "Range" code required the same attention as my original attention.
> 
> I'm gonna need someone to look over my changes closely that I'm gonna put
> forward next, because -current httpd doesn't compile on my system due to
> some changes in SSL, and I don't want to fully go -current just yet on this
> box (my only i386 box).  The following has a change in content_length from
> size_t to off_t in function server_partial_file_request() because it does 
> this:
> 
> content_length = range->end - range->start + 1;
> 
> and range->end and range->start are both off_t.
> 
> Here then the patches against -current (need review and testing):

Here is my third attempt.  Someone correctly told me that off_t is signed.
This makes it difficult to make it clean as it was before my attempts.  But
take a look at this code and see what I mean.  The below is untested.

I set content_length to 0 because it's better than having a negative value
fly around the code execution path.

-peter


? httpd.patch
Index: httpd.h
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.102
diff -u -p -u -r1.102 httpd.h
--- httpd.h 2 Dec 2015 15:13:00 -   1.102
+++ httpd.h 16 Jan 2016 04:58:16 -
@@ -597,7 +597,7 @@ const char *
 server_http_host(struct sockaddr_storage *, char *, size_t);
 char   *server_http_parsehost(char *, char *, size_t, int *);
 ssize_t server_http_time(time_t, char *, size_t);
-int server_log_http(struct client *, unsigned int, size_t);
+int server_log_http(struct client *, unsigned int, off_t);
 
 /* server_file.c */
 int server_file(struct httpd *, struct client *);
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.60
diff -u -p -u -r1.60 server_file.c
--- server_file.c   3 Aug 2015 11:45:17 -   1.60
+++ server_file.c   16 Jan 2016 04:58:16 -
@@ -303,7 +303,7 @@ server_partial_file_request(struct httpd
struct media_type   *media, multipart_media;
struct range*range;
struct evbuffer *evb = NULL;
-   size_t   content_length;
+   off_tcontent_length;
int  code = 500, fd = -1, i, nranges, ret;
uint32_t boundary;
char content_range[64];
@@ -386,6 +386,9 @@ server_partial_file_request(struct httpd
"byteranges; boundary=%ud", boundary);
media = _media;
}
+
+   if (content_length < 0)
+   content_length = 0;
 
ret = server_response_http(clt, 206, media, content_length,
MINIMUM(time(NULL), st->st_mtim.tv_sec));
Index: server_http.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.103
diff -u -p -u -r1.103 server_http.c
--- server_http.c   7 Dec 2015 20:30:17 -   1.103
+++ server_http.c   16 Jan 2016 04:58:17 -
@@ -1443,7 +1443,7 @@ server_httperror_cmp(const void *a, cons
 }
 
 int
-server_log_http(struct client *clt, unsigned int code, size_t len)
+server_log_http(struct client *clt, unsigned int code, off_t len)
 {
static char  tstamp[64];
static char  ip[INET6_ADDRSTRLEN];
@@ -1504,7 +1504,7 @@ server_log_http(struct client *clt, unsi
goto done;
 
ret = evbuffer_add_printf(clt->clt_log,
-   "%s %s - %s [%s] \"%s %s%s%s%s%s\" %03d %zu\n",
+   "%s %s - %s [%s] \"%s %s%s%s%s%s\" %03d %qd\n",
srv_conf->name, ip, clt->clt_remote_user == NULL ? "-" :
user, tstamp,
server_httpmethod_byid(desc->http_method),
@@ -1552,7 +1552,7 @@ server_log_http(struct client *clt, unsi
 
ret = evbuffer_add_printf(clt->clt_log,
"%s %s - %s [%s] \"%s %s%s%s%s%s\""
-   " %03d %zu \"%s\" \"%s\"\n",
+   " %03d %qd \"%s\" \"%s\"\n",
srv_conf->name, ip, clt->clt_remote_user == NULL ? "-" :
user, tstamp,
server_httpmethod_byid(desc->http_method),

Re: httpd: patch to close TLS sockets that,fail before TLS handshake

[Joel Sing]

On Tuesday 25 August 2015 19:19:58 Edgar Pettijohn wrote:
 I was curious if this issue is fixed in -current or if there is going to
 be a patch available on the errata page?

Yes, this is fixed in -current (and will be in 5.8) - see r1.68 of server.c. 
There may be back ports/commits of various httpd fixes at some point, but at 
this stage there I'm not sure there will be errata.

Re: httpd: patch to close TLS sockets that,fail before TLS handshake

[Edgar Pettijohn]

Good enough for me.

Thanks

On 08/27/15 08:42, Joel Sing wrote:

On Tuesday 25 August 2015 19:19:58 Edgar Pettijohn wrote:

I was curious if this issue is fixed in -current or if there is going to
be a patch available on the errata page?

Yes, this is fixed in -current (and will be in 5.8) - see r1.68 of server.c.
There may be back ports/commits of various httpd fixes at some point, but at
this stage there I'm not sure there will be errata.

Re: httpd: patch to close TLS sockets that,fail before TLS handshake

[Edgar Pettijohn]

I was curious if this issue is fixed in -current or if there is going to 
be a patch available on the errata page?


Thanks

Edgar

Re: httpd: patch to close TLS sockets that fail before TLS handshake

[Joel Sing]

On Wednesday 15 July 2015 23:38:33 Jack Burton wrote:
 In 5.7-stable  -current, httpd, when listening for TLS, does not close
 the client socket when tls_accept_socket() returns any non-recoverable
 error. The problem manifests most often when a client connects but does
 not attempt TLS handshake.
 
 Steps to reproduce:
 
 * Configure httpd to listen for TLS requests
 * From a remote host, open TCP session to httpd on port 443
 * Close the TCP connection without sending any data
 * Wait at least 2 * MSL to avoid false positives
 * fstat continues to show the open socket for as long as httpd runs
 
 This causes the number of sockets httpd has opne to keep growing until
 it reaches (openfiles-cur - 7), at which point httpd stops responding
 to HTTPS requests altogether.
 
 Real world occurrence:
 
 We've seen this fairly regularly on a host with only modest HTTPS load,
 where httpd stops responding to HTTPS requests anywhere between 2 hours
 and 4 days after httpd restart.
 
 See the thread httpd stops accepting connections after a few hours on
 current on misc@ for further background and several independent reports
 of the observed behaviour.
 
 Despite the title of that thread, the same behaviour is seen in
 5.7-stable.
 
 Sorry, I don't have any hosts running -current at the moment, but I've
 written a trivial patch against 5.7-stable to treat that particular
 failure mode in the same way as was already being done for EV_TIMEOUTs.
 That fixes the issue for us here (been in place on one production host
 with a modest [2req/sec avg] load for 4 hours with no obvious
 regressions and no stale sockets -- previously we were getting at least
 several stale sockets appearing every hour). The good folks on misc@
 suggested I should post my patch to tech@, so here it is:
 
 --- usr.sbin/httpd/server.c.origWed Jul 15 20:40:16 2015
 +++ usr.sbin/httpd/server.c Wed Jul 15 20:50:15 2015
 @@ -932,6 +932,7 @@ server_accept_tls(int fd, short event, void *arg)
 struct client *clt = (struct client *)arg;
 struct server *srv = (struct server *)clt-clt_srv;
 int ret;
 + char *errmsg;
 
 if (event == EV_TIMEOUT) {
 server_close(clt, TLS accept timeout);
 @@ -952,8 +953,13 @@ server_accept_tls(int fd, short event, void *arg)
 server_accept_tls, clt-clt_tv_start,
 srv-srv_conf.timeout, clt);
 } else if (ret != 0) {
 - log_warnx(%s: TLS accept failed - %s, __func__,
 - tls_error(srv-srv_tls_ctx));
 + if (asprintf(errmsg, %s: TLS accept failed - %s,
 + __func__, tls_error(srv-srv_tls_ctx))  0) {
 + server_close(clt, server_accept_tls: TLS accept failed);
 + } else {
 + server_close(clt, errmsg);
 + free(errmsg);
 + }
 return;
 }
 
 This is the first time I've posted a patch for OpenBSD, so if I've erred
 in style or method please correct me and I'll try to do better next
 time.

Thanks for finding this and providing a diff. I've just committed a slightly 
different version, which simply adds the missing server_close() call - the 
version above will result in additional noise in the logs (both the function 
name and the full details from tls_error()), which we'd rather avoid.

Re: httpd: patch to close TLS sockets that fail before TLS handshake

[Jack Burton]

On Wed, 2015-07-15 at 23:38 +0930, Jack Burton wrote: 
 Sorry, I don't have any hosts running -current at the moment, but I've
 written a trivial patch against 5.7-stable to treat that particular
 failure mode in the same way as was already being done for EV_TIMEOUTs.
 That fixes the issue for us here (been in place on one production host
 with a modest [2req/sec avg] load for 4 hours with no obvious
 regressions and no stale sockets -- previously we were getting at least
 several stale sockets appearing every hour). The good folks on misc@
 suggested I should post my patch to tech@, so here it is:

Hmm, that's a bit painful to read -- it seems my mail client collapsed
all the tabs into single spaces. I'll try again with tabs expanded:

--- usr.sbin/httpd/server.c.origWed Jul 15 20:40:16 2015
+++ usr.sbin/httpd/server.c Wed Jul 15 20:50:15 2015
@@ -932,6 +932,7 @@ server_accept_tls(int fd, short event, void *arg)
struct client *clt = (struct client *)arg;
struct server *srv = (struct server *)clt-clt_srv;
int ret;
+   char *errmsg;

if (event == EV_TIMEOUT) {
server_close(clt, TLS accept timeout);
@@ -952,8 +953,13 @@ server_accept_tls(int fd, short event, void *arg)
server_accept_tls, clt-clt_tv_start,
srv-srv_conf.timeout, clt);
} else if (ret != 0) {
-   log_warnx(%s: TLS accept failed - %s, __func__,
-   tls_error(srv-srv_tls_ctx));
+   if (asprintf(errmsg, %s: TLS accept failed - %s,
+   __func__, tls_error(srv-srv_tls_ctx))  0) {
+   server_close(clt, server_accept_tls: TLS accept
failed);
+   } else {
+   server_close(clt, errmsg);
+   free(errmsg);
+   }
return;
}