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

httpd: patch for portability asprintf use

[Hiltjo Posthuma]

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 */

--

Kind regards,
Hiltjo