Re: syslog_r() should use strerror_r()

[Alexander Bluhm]

On Wed, Oct 19, 2016 at 09:52:13AM -0600, Todd C. Miller wrote:
> On Wed, 19 Oct 2016 09:11:36 -0600, "Todd C. Miller" wrote:
> 
> > Currently, syslog_r() avoids using strerror() since it is not
> > reentrant.  We have a reentrant strerror_r() so let's use it.
> 
> Since strerror_r() always fills in the buffer there is no need to
> check the return value.  If saved_errno is not a valid error number,
> strerror_r() will use "Unknown error: %d".

OK bluhm@

> 
>  - todd
> 
> Index: lib/libc/gen/syslog_r.c
> ===
> RCS file: /cvs/src/lib/libc/gen/syslog_r.c,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 syslog_r.c
> --- lib/libc/gen/syslog_r.c   27 Mar 2016 16:28:56 -  1.15
> +++ lib/libc/gen/syslog_r.c   19 Oct 2016 15:48:41 -
> @@ -138,18 +138,13 @@ __vsyslog_r(int pri, struct syslog_data 
>   }
>   }
>  
> - /* strerror() is not reentrant */
> -
>   for (t = fmt_cpy, fmt_left = FMT_LEN; (ch = *fmt); ++fmt) {
>   if (ch == '%' && fmt[1] == 'm') {
> + char ebuf[NL_TEXTMAX];
> +
>   ++fmt;
> - if (reentrant) {
> - prlen = snprintf(t, fmt_left, "Error %d",
> - saved_errno); 
> - } else {
> - prlen = snprintf(t, fmt_left, "%s",
> - strerror(saved_errno)); 
> - }
> + (void)strerror_r(saved_errno, ebuf, sizeof(ebuf));
> + prlen = snprintf(t, fmt_left, "%s", ebuf);
>   if (prlen < 0)
>   prlen = 0;
>   if (prlen >= fmt_left)

Re: syslog_r() should use strerror_r()

[Alexander Bluhm]

On Wed, Oct 19, 2016 at 09:34:16AM -0600, Theo de Raadt wrote:
> Inside strerror_r, I'm unsure why there is a save_errno dance.

That is from the time when we had national language support.

ok?

bluhm

Index: lib/libc/string/strerror_r.c
===
RCS file: /data/mirror/openbsd/cvs/src/lib/libc/string/strerror_r.c,v
retrieving revision 1.12
diff -u -p -r1.12 strerror_r.c
--- lib/libc/string/strerror_r.c25 Oct 2015 10:22:09 -  1.12
+++ lib/libc/string/strerror_r.c19 Oct 2016 15:57:45 -
@@ -85,15 +85,13 @@ __num2string(int num, int sign, int seti
 int
 strerror_r(int errnum, char *strerrbuf, size_t buflen)
 {
-   int save_errno;
int ret_errno;
 
-   save_errno = errno;
-
ret_errno = __num2string(errnum, 1, 1, strerrbuf, buflen,
sys_errlist, sys_nerr, UPREFIX);
 
-   errno = ret_errno ? ret_errno : save_errno;
+   if (ret_errno)
+   errno = ret_errno;
return (ret_errno);
 }
 DEF_WEAK(strerror_r);

Re: syslog_r() should use strerror_r()

[Todd C. Miller]

On Wed, 19 Oct 2016 09:11:36 -0600, "Todd C. Miller" wrote:

> Currently, syslog_r() avoids using strerror() since it is not
> reentrant.  We have a reentrant strerror_r() so let's use it.

Since strerror_r() always fills in the buffer there is no need to
check the return value.  If saved_errno is not a valid error number,
strerror_r() will use "Unknown error: %d".

 - todd

Index: lib/libc/gen/syslog_r.c
===
RCS file: /cvs/src/lib/libc/gen/syslog_r.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 syslog_r.c
--- lib/libc/gen/syslog_r.c 27 Mar 2016 16:28:56 -  1.15
+++ lib/libc/gen/syslog_r.c 19 Oct 2016 15:48:41 -
@@ -138,18 +138,13 @@ __vsyslog_r(int pri, struct syslog_data 
}
}
 
-   /* strerror() is not reentrant */
-
for (t = fmt_cpy, fmt_left = FMT_LEN; (ch = *fmt); ++fmt) {
if (ch == '%' && fmt[1] == 'm') {
+   char ebuf[NL_TEXTMAX];
+
++fmt;
-   if (reentrant) {
-   prlen = snprintf(t, fmt_left, "Error %d",
-   saved_errno); 
-   } else {
-   prlen = snprintf(t, fmt_left, "%s",
-   strerror(saved_errno)); 
-   }
+   (void)strerror_r(saved_errno, ebuf, sizeof(ebuf));
+   prlen = snprintf(t, fmt_left, "%s", ebuf);
if (prlen < 0)
prlen = 0;
if (prlen >= fmt_left)

Re: syslog_r() should use strerror_r()

[Todd C. Miller]

On Wed, 19 Oct 2016 09:34:16 -0600, "Theo de Raadt" wrote:

> Inside strerror_r, I'm unsure why there is a save_errno dance.

I don't see anything called by strerror_r() that would touch errno
so we could get rid of the save_errno dance and just do:

if (ret_errno)
errno = ret_errno;

 - todd

Re: syslog_r() should use strerror_r()

[Todd C. Miller]

On Wed, 19 Oct 2016 17:29:18 +0200, Mark Kettenis wrote:

> Perhaps add a comment explicitly stating that OpenBSD's strerror_r()
> *is* reentrant?

I thought that was implicit in the name.

 - todd

Re: syslog_r() should use strerror_r()

[Theo de Raadt]

> > Currently, syslog_r() avoids using strerror() since it is not
> > reentrant.  We have a reentrant strerror_r() so let's use it.
> > 
> > OK?
> 
> Perhaps add a comment explicitly stating that OpenBSD's strerror_r()
> *is* reentrant?

Also, signal-handler safe.

We've done something a little crazy -- pushing hard at libc to make
our syslog_r function threadsafe + reentrant + and signal-handler
safe, as long as the format string doesn't contain floating point.

So Mark is right, any public layers below syslog_r should declare
their safety conditions also, otherwise people may puzzle.

Inside strerror_r, I'm unsure why there is a save_errno dance.

Re: syslog_r() should use strerror_r()

[Mark Kettenis]

> From: "Todd C. Miller" 
> Date: Wed, 19 Oct 2016 09:11:36 -0600
> 
> Currently, syslog_r() avoids using strerror() since it is not
> reentrant.  We have a reentrant strerror_r() so let's use it.
> 
> OK?

Perhaps add a comment explicitly stating that OpenBSD's strerror_r()
*is* reentrant?

> Index: lib/libc/gen/syslog_r.c
> ===
> RCS file: /cvs/src/lib/libc/gen/syslog_r.c,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 syslog_r.c
> --- lib/libc/gen/syslog_r.c   27 Mar 2016 16:28:56 -  1.15
> +++ lib/libc/gen/syslog_r.c   19 Oct 2016 15:06:48 -
> @@ -138,17 +138,16 @@ __vsyslog_r(int pri, struct syslog_data 
>   }
>   }
>  
> - /* strerror() is not reentrant */
> -
>   for (t = fmt_cpy, fmt_left = FMT_LEN; (ch = *fmt); ++fmt) {
>   if (ch == '%' && fmt[1] == 'm') {
> + char ebuf[NL_TEXTMAX];
> +
>   ++fmt;
> - if (reentrant) {
> + if (strerror_r(saved_errno, ebuf, sizeof(ebuf)) != 0) {
>   prlen = snprintf(t, fmt_left, "Error %d",
>   saved_errno); 
>   } else {
> - prlen = snprintf(t, fmt_left, "%s",
> - strerror(saved_errno)); 
> + prlen = snprintf(t, fmt_left, "%s", ebuf);
>   }
>   if (prlen < 0)
>   prlen = 0;
> 
>