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

sure, OK florian

On 2024-05-21 14:21 +02, Martijn van Duren <openbsd+tech@list.imperialat.at> wrote:
> 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));
>  }
>  
>

-- 
In my defence, I have been left unsupervised.