From: Jan Klemkow <jan@openbsd.org>
Subject: Re: newsyslog: use localtime_r(3) w/ error handling
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 15 Sep 2025 19:44:51 +0200

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.

ok?

bye,
Jan

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