From: Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject: Re: localtime(3) / gmtime(3) error checking for bin / libexec / sbin
To: tech <tech@openbsd.org>
Cc: Florian Obser <florian@openbsd.org>
Date: Tue, 21 May 2024 14:21:06 +0200

On Sun, 2024-04-28 at 12:03 +0200, Florian Obser wrote:
> Mostly compile tested only. Most of these probably can't happen. At
> least I don't expect the kernel to work well if the time is so far in
> the future that the year can't be represented by a struct tm. But I
> suppose just adding the error check is easier than arguing why it's not
> needed.
> 
> Others catch operator errors, e.g.:
> $ date -r 1714200723888079566 20240428090914
> Segmentation fault (core dumped)
> 
> Comments, OKs?
> 
> I'm happy to take OKs for individual chunks and drop chunks that are
> deemed too silly. Depending how this goes I'll do usr.bin and usr.sbin
> as time permits.
> 

Hello,

Bit behind on my mail.

I'm okay with keeping this change as is, but wouldn't it be better if we
throw an error here and be a little more noisy about it instead of
returning a nonsensical value?

I went with log_warnx() here, since our localtime doesn't touch errno,
but note that POSIX[0] states that it does.

martijn@

[0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html

Index: mib.c
===================================================================
RCS file: /cvs/src/libexec/snmpd/snmpd_metrics/mib.c,v
retrieving revision 1.8
diff -u -p -r1.8 mib.c
--- mib.c	28 Apr 2024 16:42:53 -0000	1.8
+++ mib.c	21 May 2024 12:19:33 -0000
@@ -296,29 +296,31 @@ mib_hrsystemdate(struct agentx_varbind *
 	int		 tzoffset;
 	unsigned short	 year;
 
-	memset(s, 0, sizeof(s));
 	(void)time(&now);
 	ptm = localtime(&now);
 
-	if (ptm != NULL) {
-		year = htons(ptm->tm_year + 1900);
-		memcpy(s, &year, 2);
-		s[2] = ptm->tm_mon + 1;
-		s[3] = ptm->tm_mday;
-		s[4] = ptm->tm_hour;
-		s[5] = ptm->tm_min;
-		s[6] = ptm->tm_sec;
-		s[7] = 0;
+	if (ptm == NULL) {
+		log_warnx("localtime");
+		agentx_varbind_error(vb);
+		return;
+	}
+	year = htons(ptm->tm_year + 1900);
+	memcpy(s, &year, 2);
+	s[2] = ptm->tm_mon + 1;
+	s[3] = ptm->tm_mday;
+	s[4] = ptm->tm_hour;
+	s[5] = ptm->tm_min;
+	s[6] = ptm->tm_sec;
+	s[7] = 0;
 
-		tzoffset = ptm->tm_gmtoff;
-		if (tzoffset < 0)
-			s[8] = '-';
-		else
-			s[8] = '+';
+	tzoffset = ptm->tm_gmtoff;
+	if (tzoffset < 0)
+		s[8] = '-';
+	else
+		s[8] = '+';
 
-		s[9] = abs(tzoffset) / 3600;
-		s[10] = (abs(tzoffset) - (s[9] * 3600)) / 60;
-	}
+	s[9] = abs(tzoffset) / 3600;
+	s[10] = (abs(tzoffset) - (s[9] * 3600)) / 60;
 	agentx_varbind_nstring(vb, s, sizeof(s));
 }