From: Christian Schulte <cs@schulte.it>
Subject: Re: httpd: add timestamps to fastcgi error log
To: tech@openbsd.org
Date: Mon, 2 Nov 2026 08:53:03 +0100

Am 01.11.2026 um 20:46 schrieb Rafael Sadowski:
> commit cc7e31596c708775558a0b857170bb94fce407d0
> Author: Rafael Sadowski <rafael@sizeofvoid.org>
> Date:   Sun Nov 1 15:18:04 2026 +0100
> 
>     httpd: add timestamps to fastcgi error log
> 
> diff --git a/server.c b/server.c
> index 9975a1c..750e9ca 100644
> --- a/server.c
> +++ b/server.c
> @@ -1541,4 +1541,21 @@ server_client_cmp(struct client *a, struct client *b)
>  	return ((int)a->clt_id - b->clt_id);
>  }
>  
> +int
> +server_log_time(char *buf, size_t len, const char *format)
> +{
> +	time_t		 t;
> +	struct tm	 tm;
> +
> +	if ((t = time(NULL)) == -1)
> +		return (-1);
> +	if (localtime_r(&t, &tm) == NULL)
> +		return (-1);
> +	if (strftime(buf, len,
> +	    format == NULL ? "%d/%b/%Y:%H:%M:%S %z" : format, &tm) == 0)
> +		return (-1);
> +
> +	return (0);
> +}
> +

Maybe make the format argument mandatory here and move the format to
where it is needed (see below) instead of introducing a questionable
default here.

> diff --git a/server_http.c b/server_http.c
> index 2c4bb74..bc7e90c 100644
> --- a/server_http.c
> +++ b/server_http.c
> @@ -2042,13 +2042,11 @@ replace_var(char *str, const char *var, const char *repl)
>  int
>  server_log_http(struct client *clt, unsigned int code, size_t len)
>  {
> -	static char		 tstamp[64];
>  	static char		 ip[INET6_ADDRSTRLEN];
> -	time_t			 t;
>  	struct kv		 key, *agent, *referrer, *xff, *xfp;
> -	struct tm		*tm;
>  	struct server_config	*srv_conf;
>  	struct http_descriptor	*desc;
> +	char			 tstamp[64];
>  	int			 ret = -1;
>  	char			*user = NULL;
>  	char			*path = NULL;
> @@ -2065,11 +2063,7 @@ server_log_http(struct client *clt, unsigned int code, size_t len)
>  	if ((desc = clt->clt_descreq) == NULL)
>  		return (-1);
>  
> -	if ((t = time(NULL)) == -1)
> -		return (-1);
> -	if ((tm = localtime(&t)) == NULL)
> -		return (-1);
> -	if (strftime(tstamp, sizeof(tstamp), "%d/%b/%Y:%H:%M:%S %z", tm) == 0)
> +	if (server_log_time(tstamp, sizeof(tstamp), NULL) == -1)

So that passing NULL to server_log_time here intentionally would
segfault and the developer using that function has to make up his mind
about the format to use like:

if (sever_log_time(tstamp, sizeof(tstamp), "%d/%b/%Y:%H:%M:%S %z"))

As a side note one could consider doing something like
getenv(HTTPD_TIME_FORMAT) or something like that so that an operator
could change the preferred format to use in some login.conf file. With
added input validation, of course.

Regards,
-- 
Christian