[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From f3514f26297e884a00d4fb29191bd9978eb03e7b Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert at cs.ucla.edu>
> Date: Sat, 11 Mar 2023 00:42:29 -0800
> Subject: [PATCH 6/8] Fix crash with large timestamps
> 
> * libmisc/date_to_str.c (date_to_str): Do not crash if gmtime
> returns NULL because the timestamp is far in the future.
> Instead, use a dummy struct tm * to pacify any pedantic runtime.
> Simplify by always calling strftime, instead of sometimes strftime
> and sometimes strlcpy.
> 
> Signed-off-by: Paul Eggert <eggert at cs.ucla.edu>
> ---
>  libmisc/date_to_str.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/libmisc/date_to_str.c b/libmisc/date_to_str.c
> index f3b9dc76..0840c38c 100644
> --- a/libmisc/date_to_str.c
> +++ b/libmisc/date_to_str.c
> @@ -35,13 +35,16 @@
>  
>  void date_to_str (size_t size, char buf[size], long date)
>  {
> -	time_t t;
> +	time_t t = date;
> +	struct tm *tm = gmtime (&t);
>  
> -	t = date;
> -	if (date < 0) {
> -		(void) strlcpy (buf, "never", size);
> -	} else {
> -		(void) strftime (buf, size, "%Y-%m-%d", gmtime (&t));
> -		buf[size - 1] = '\0';
> -	}

Please split the patch into two: one that fixes the UB,
and another that removes the strlcpy(3) calls.  They
can be done independently, and I'm not convinced by your
measurement of simplicity :)


On 3/12/23 02:38, Paul Eggert wrote:
> It depends on how one measures simplicity. The reader will need to know 
> strftime's API regardless; requiring the reader to also know strlcpy's 
> API makes the reader's job harder.

I expect readers to easily learn that from the relevant man page :)
Once you know what strlcpy(3) is, that is, a function that copies a
string with truncation, it's straightforward to read.  Knowing
strftime(3)'s details is more obscure for those who are not time
nerds :-)

> 
> Also, it's less machine code to call just the one function (if one cares 
> about simplicity of debugging 😄.

Heh, I could count with my fingers the number of days I've used
gdb(1) in my entire life :D.  Hail fprintf(3) and stderr!  Which
BTW benefit from expanded conditionals, rather than ternary ops.

> 
> If you still prefer calling two different functions instead of just one, 
> feel free to modify it to use plain strcpy. strlcpy isn't needed here as 
> the destination buffers are all big enough.

Do we know that?  The API receives a size as a parameter, which
could perfectly be 1 (well, I know we're not going to do that,
but since it's a generic API, I wouldn't rely on current sizes).

Cheers,

Alex

> To be honest though I like 
> it the way it is (though it could use a comment; I'll add that).

> +	/* A dummy whose address can be passed to strftime to avoid
> +	   undefined behavior.  It's OK for it to be uninitialized
> +	   since no conversion specs are used.  */
> +	struct tm dummy;
> +
> +	(void) strftime (buf, size,
> +			 date < 0 ? "never" : tm ? "%Y-%m-%d" : "future",
> +			 tm ? tm : &dummy);
> +	buf[size - 1] = '\0';
>  }
> -- 
> 2.37.2
> 


-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-shadow-devel/attachments/20230312/9dea154b/attachment-0001.sig>