[PATCH] implement the LOG_PERROR option in syslog

[PATCH] implement the LOG_PERROR option in syslog

[Clément Vasseur]

Since musl defines this option in its header file, maybe it should
actually do something.
---
 src/misc/syslog.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/misc/syslog.c b/src/misc/syslog.c
index 57f1d75..2b0c73b 100644
--- a/src/misc/syslog.c
+++ b/src/misc/syslog.c
@@ -79,7 +79,7 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	char buf[256];
 	int errno_save = errno;
 	int pid;
-	int l, l2;
+	int l, l1, l2, l3;
 
 	if (log_fd < 0) {
 		__openlog();
@@ -93,14 +93,16 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	strftime(timebuf, sizeof timebuf, "%b %e %T", &tm);
 
 	pid = (log_opt & LOG_PID) ? getpid() : 0;
-	l = snprintf(buf, sizeof buf, "<%d>%s %s%s%.0d%s: ",
-		priority, timebuf, log_ident, "["+!pid, pid, "]"+!pid);
+	l1 = snprintf(buf, sizeof buf, "<%d>%s ", priority, timebuf);
+	l2 = snprintf(buf+l1, sizeof buf - l1, "%s%s%.0d%s: ",
+		log_ident, "["+!pid, pid, "]"+!pid);
 	errno = errno_save;
-	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
-	if (l2 >= 0) {
-		if (l2 >= sizeof buf - l) l = sizeof buf - 1;
-		else l += l2;
+	l3 = vsnprintf(buf+l1+l2, sizeof buf - (l1+l2), message, ap);
+	if (l3 >= 0) {
+		if (l3 >= sizeof buf - (l1+l2)) l = sizeof buf - 1;
+		else l = l1+l2+l3;
 		if (buf[l-1] != '\n') buf[l++] = '\n';
+		if (log_opt & LOG_PERROR) write(2, buf+l1, l-l1);
 		send(log_fd, buf, l, 0);
 	}
 }
-- 
2.0.1

Re: [PATCH] implement the LOG_PERROR option in syslog

[Rich Felker]

On Wed, Jul 09, 2014 at 02:42:12PM +0200, Clément Vasseur wrote:
> Since musl defines this option in its header file, maybe it should
> actually do something.
> ---
>  src/misc/syslog.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/misc/syslog.c b/src/misc/syslog.c
> index 57f1d75..2b0c73b 100644
> --- a/src/misc/syslog.c
> +++ b/src/misc/syslog.c
> @@ -79,7 +79,7 @@ static void _vsyslog(int priority, const char *message, va_list ap)
>  	char buf[256];
>  	int errno_save = errno;
>  	int pid;
> -	int l, l2;
> +	int l, l1, l2, l3;
>  
>  	if (log_fd < 0) {
>  		__openlog();
> @@ -93,14 +93,16 @@ static void _vsyslog(int priority, const char *message, va_list ap)
>  	strftime(timebuf, sizeof timebuf, "%b %e %T", &tm);
>  
>  	pid = (log_opt & LOG_PID) ? getpid() : 0;
> -	l = snprintf(buf, sizeof buf, "<%d>%s %s%s%.0d%s: ",
> -		priority, timebuf, log_ident, "["+!pid, pid, "]"+!pid);
> +	l1 = snprintf(buf, sizeof buf, "<%d>%s ", priority, timebuf);
> +	l2 = snprintf(buf+l1, sizeof buf - l1, "%s%s%.0d%s: ",
> +		log_ident, "["+!pid, pid, "]"+!pid);
>  	errno = errno_save;
> -	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
> -	if (l2 >= 0) {
> -		if (l2 >= sizeof buf - l) l = sizeof buf - 1;
> -		else l += l2;
> +	l3 = vsnprintf(buf+l1+l2, sizeof buf - (l1+l2), message, ap);
> +	if (l3 >= 0) {
> +		if (l3 >= sizeof buf - (l1+l2)) l = sizeof buf - 1;
> +		else l = l1+l2+l3;
>  		if (buf[l-1] != '\n') buf[l++] = '\n';
> +		if (log_opt & LOG_PERROR) write(2, buf+l1, l-l1);
>  		send(log_fd, buf, l, 0);
>  	}
>  }

This patch looks a lot more invasive than it needs to be, and (as
discussed off-list with Alexander's concerns that there might be a
buffer overflow here) makes it a lot less obvious that the code is
safe. The reason for the two separate snprintf/vsnprintf calls in the
original version of the file was purely because C does not allow
merging variadic argument lists. It's ugly as-is because it violates
the principle that snprintf should be used to construct the entire
string in one operation rather than in multiple calls with error-prone
arithmetic on the remaining space, but there's no way to avoid it.

Anyway, for adding the feature you want, where you just want to know
the number of initial bytes to skip writing to stderr, I would just
use %n to get the offset.

Rich

Re: [PATCH] implement the LOG_PERROR option in syslog

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Fri, Jul 11, 2014 at 12:49:50AM -0400, Rich Felker wrote:
> Anyway, for adding the feature you want, where you just want to know
> the number of initial bytes to skip writing to stderr, I would just
> use %n to get the offset.

Attached is a patch to do this. I'll commit it soon if nobody notices
any problems right away.

Rich

[-- Attachment #2: syslog_perror.diff --]
[-- Type: text/plain, Size: 1072 bytes --]

diff --git a/src/misc/syslog.c b/src/misc/syslog.c
index 57f1d75..d2ea4aa 100644
--- a/src/misc/syslog.c
+++ b/src/misc/syslog.c
@@ -80,6 +80,7 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	int errno_save = errno;
 	int pid;
 	int l, l2;
+	int hlen;
 
 	if (log_fd < 0) {
 		__openlog();
@@ -93,8 +94,8 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	strftime(timebuf, sizeof timebuf, "%b %e %T", &tm);
 
 	pid = (log_opt & LOG_PID) ? getpid() : 0;
-	l = snprintf(buf, sizeof buf, "<%d>%s %s%s%.0d%s: ",
-		priority, timebuf, log_ident, "["+!pid, pid, "]"+!pid);
+	l = snprintf(buf, sizeof buf, "<%d>%s %n%s%s%.0d%s: ",
+		priority, timebuf, &hlen, log_ident, "["+!pid, pid, "]"+!pid);
 	errno = errno_save;
 	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
 	if (l2 >= 0) {
@@ -102,6 +103,7 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 		else l += l2;
 		if (buf[l-1] != '\n') buf[l++] = '\n';
 		send(log_fd, buf, l, 0);
+		if (log_opt & LOG_PERROR) write(2, buf+hlen, l-hlen);
 	}
 }
 

Re: [PATCH] implement the LOG_PERROR option in syslog

[Clément Vasseur]

On 2014-07-12, Rich Felker <dalias@libc.org> wrote:
>
> [-- Type: text/plain, Encoding:  --]
>
> On Fri, Jul 11, 2014 at 12:49:50AM -0400, Rich Felker wrote:
>> Anyway, for adding the feature you want, where you just want to know
>> the number of initial bytes to skip writing to stderr, I would just
>> use %n to get the offset.
>
> Attached is a patch to do this. I'll commit it soon if nobody notices
> any problems right away.

Looks good to me.
Thanks!

Re: [PATCH] implement the LOG_PERROR option in syslog

[Rich Felker]

On Fri, Jul 11, 2014 at 08:55:25PM -0400, Rich Felker wrote:
> On Fri, Jul 11, 2014 at 12:49:50AM -0400, Rich Felker wrote:
> > Anyway, for adding the feature you want, where you just want to know
> > the number of initial bytes to skip writing to stderr, I would just
> > use %n to get the offset.
> 
> Attached is a patch to do this. I'll commit it soon if nobody notices
> any problems right away.
> [...]
>  		else l += l2;
>  		if (buf[l-1] != '\n') buf[l++] = '\n';
>  		send(log_fd, buf, l, 0);
> +		if (log_opt & LOG_PERROR) write(2, buf+hlen, l-hlen);
>  	}

I think I'll change this to dprintf. Using a single write like this
without looping on partial writes is not guaranteed to work. dprintf
is overkill, but we pull in the printf framework anyway so it doesn't
hurt to use it.

Rich

Re: [PATCH] implement the LOG_PERROR option in syslog

[Laurent Bercot]

On 12/07/2014 02:15, Rich Felker wrote:
> I think I'll change this to dprintf. Using a single write like this
> without looping on partial writes is not guaranteed to work. dprintf
> is overkill, but we pull in the printf framework anyway so it doesn't
> hurt to use it.

  Don't you have a wrapper for write() that loops around partial writes
without pulling in printf ? I'd have thought it was needed in more than
one place in the libc, stdio being only one of them.

-- 
  Laurent

Re: [PATCH] implement the LOG_PERROR option in syslog

[Rich Felker]

On Sat, Jul 12, 2014 at 09:33:21AM +0100, Laurent Bercot wrote:
> On 12/07/2014 02:15, Rich Felker wrote:
> >I think I'll change this to dprintf. Using a single write like this
> >without looping on partial writes is not guaranteed to work. dprintf
> >is overkill, but we pull in the printf framework anyway so it doesn't
> >hurt to use it.
> 
>  Don't you have a wrapper for write() that loops around partial writes
> without pulling in printf ? I'd have thought it was needed in more than
> one place in the libc,

So far none have come up. There's very little code in libc that writes
to files. A few places (dns, syslog) write to sockets, but they use
send on datagram sockets in a manner where success or failure has to
be atomic (you can't retry on partial write with datagrams). There are
also a couple of places (posix_spawn comes to mind) that write to
internal pipes, but the same applies: writes < PIPE_BUF are atomic.

Anyway, actually it would be a good idea to check for invalid uses of
read or write without retry in musl, but I doubt there are any serious
ones left.

> stdio being only one of them.

stdio uses writev in a more complex way that doesn't easily generalize
to generic usage like this, since it's working with both a FILE buffer
and a secondary buffer provided by the caller.

Rich