Re: Proposed syslog patch [Re: [musl] Further bugs in syslog()]

[Rich Felker <dalias@aerifal.cx>]

On Sat, Mar 23, 2013 at 05:17:55PM +0100, Szabolcs Nagy wrote:
> looks ok, one nitpick
> 
> * Rich Felker <dalias@aerifal.cx> [2013-03-23 00:05:58 -0400]:
> >  void openlog(const char *ident, int opt, int facility)
> > @@ -59,7 +54,14 @@ void openlog(const char *ident, int opt, int facility)
> >  	int cs;
> >  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> >  	LOCK(lock);
> > -	__openlog(ident, opt, facility);
> > +
> > +	free(log_ident);
> > +	log_ident = strdup(ident);
> 
> this can fail and then log_ident = 0

Yes, this was the intent; there's no way to report errors, and being
in a status of NULL log_ident is the same as the case where it was
never set at all. However I agree with your reasoning below...

> ....
> > @@ -90,14 +89,14 @@ static void _vsyslog(int priority, const char *message, va_list ap)
> >  		priority, timebuf,
> >  		log_ident ? log_ident : "",
> >  		"["+!pid, pid, "]"+!pid);
> > +	if (l >= sizeof buf) return;
> 
> if ident is longer than buf, then syslog will silently fail here,
> but if ident is so big that strdup fails above it does not fail here

That it violates the principle of least surprise that moderately long
values of log_ident will prevent any log message from appearing, but
insanely long ones will be silently replaced with a blank string.

> maybe we should limit ident to a sensible value and truncate it
> (i dont know if posix allows that but seems more consistent
> and then no dynamic allocation is needed in openlog)

POSIX provides no way to inspect the output that's produced, so there
are no testable conformance requirements on the output...

I think I like your approach: just using a small fixed-size buffer.

Rich