From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: newsyslog: use localtime_r(3) w/ error handling
To: Jan Klemkow <j.klemkow@wemelug.de>
Cc: tech@openbsd.org
Date: Mon, 15 Sep 2025 20:08:39 +0200

On Mon, Sep 15, 2025 at 07:44:51PM +0200, Jan Klemkow wrote:
> On Mon, Sep 15, 2025 at 12:39:27AM +0200, Alexander Bluhm wrote:
> > > On 2025/09/14 14:57, Jan Klemkow wrote:
> > > > -   tmp = localtime(&timenow);
> > > > -   tm = *tmp;
> > 
> > The old code created a copy of the libc buffer.  I guess modifying
> > the libc buffer does not harm, but it feels a bit strange.  Could
> > you use "struct tm tm; localtime_r(&timenow, &tm)" to avoid writing
> > to libc memory?
> 
> The following diff replaces localtime(3) with localtime_r(3) in both
> time parsing functions.  Also, the return code it checked now, with
> proper error handling.
> 
> > > > > > +	if (s != t) {
> > > > > > +		switch (t == NULL ? strlen(s) : t - s) {
> > > > > > +		case 8: strlcat(format, "%C", sizeof format);
> > > > > > +		case 6: strlcat(format, "%y", sizeof format);
> > > > > > +		case 4: strlcat(format, "%m", sizeof format);
> > > > > > +		case 2: strlcat(format, "%d", sizeof format);
> > > > > >  		case 0:
> > > 
> > > style(9) asks for /* FALLTHROUGH */ comments and I think that makes
> > > sense.
> > 
> > Here we have a solid block of composing format strings.  I doubt
> > that /* FALLTHROUGH */ increases readability.
> 
> I in my opinion the FALLTHROUGH comments are not helpful here.

As they fit on a line, they don't look as ugly as expected.
As they are commited now, I would keep them as they are.

> ok?

Please remove the unused tmp variable from parseDWM().
I wonder what happens when you build with WARNINGS=yes

with that OK bluhm@

> Index: newsyslog.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/newsyslog/newsyslog.c,v
> diff -u -p -r1.118 newsyslog.c
> --- newsyslog.c	14 Sep 2025 21:43:58 -0000	1.118
> +++ newsyslog.c	15 Sep 2025 17:27:40 -0000
> @@ -1179,19 +1179,21 @@ time_t
>  parse8601(char *s)
>  {
>  	char		 format[16] = { 0 };
> -	struct tm	*tm;
> +	struct tm	 tm;
>  	char		*t;
>  
> -	tm = localtime(&timenow);
> -	tm->tm_hour = tm->tm_min = tm->tm_sec = 0;
> +	if (localtime_r(&timenow, &tm) == NULL)
> +		return -1;
> +
> +	tm.tm_hour = tm.tm_min = tm.tm_sec = 0;
>  	t = strchr(s, 'T');
>  
>  	if (s != t) {
>  		switch (t == NULL ? strlen(s) : t - s) {
> -		case 8: strlcat(format, "%C", sizeof format); /* FALLTHROUGH */
> -		case 6: strlcat(format, "%y", sizeof format); /* FALLTHROUGH */
> -		case 4: strlcat(format, "%m", sizeof format); /* FALLTHROUGH */
> -		case 2: strlcat(format, "%d", sizeof format); /* FALLTHROUGH */
> +		case 8: strlcat(format, "%C", sizeof format);
> +		case 6: strlcat(format, "%y", sizeof format);
> +		case 4: strlcat(format, "%m", sizeof format);
> +		case 2: strlcat(format, "%d", sizeof format);
>  		case 0:
>  			break;
>  		default:
> @@ -1203,9 +1205,9 @@ parse8601(char *s)
>  		strlcat(format, "T", sizeof format);
>  
>  		switch (strlen(t)) {
> -		case 7: strlcat(format, "%H", sizeof format); /* FALLTHROUGH */
> -		case 5: strlcat(format, "%M", sizeof format); /* FALLTHROUGH */
> -		case 3: strlcat(format, "%S", sizeof format); /* FALLTHROUGH */
> +		case 7: strlcat(format, "%H", sizeof format);
> +		case 5: strlcat(format, "%M", sizeof format);
> +		case 3: strlcat(format, "%S", sizeof format);
>  		case 1:
>  			break;
>  		default:
> @@ -1213,10 +1215,10 @@ parse8601(char *s)
>  		}
>  	}
>  
> -	if (strptime(s, format, tm) == NULL)
> +	if (strptime(s, format, &tm) == NULL)
>  		return -1;
>  
> -	return mktime(tm);
> +	return mktime(&tm);
>  }
>  
>  /*-
> @@ -1242,8 +1244,8 @@ parseDWM(char *s)
>  	char *t;
>  	long l;
>  
> -	tmp = localtime(&timenow);
> -	tm = *tmp;
> +	if (localtime_r(&timenow, &tm) == NULL)
> +		return -1;
>  
>  	/* set no. of days per month */
>