Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t

["Érico Nogueira" <ericonr@disroot.org>]

Hi!

On Fri Nov 19, 2021 at 3:15 PM -03, Alexander Kanavin via Elfutils-devel wrote:
> From: Alexander Kanavin <alex.kanavin@gmail.com>
>
> Use intmax_t to print time_t
>
> time_t is platform dependent and some of architectures e.g.
> x32, riscv32, arc use 64bit time_t even while they are 32bit
> architectures, therefore directly using integer printf formats will not
> work portably, use intmax_t to typecast time_t into printf family of
> functions

For what it's worth, most of the time64 support patches that I have seen
use "%lld" and `long long` as the type for portable representation of
time, instead of intmax_t, but each should work just as well as the
other.

Might be worth mentioning in the commit that musl 1.2.0 and above uses
time64 on all platforms, and that glibc 2.34 has added support for
time64 (which might be enabled by distro-wide CFLAGS), with possibly
2.35 or 2.36 making it enabled by default.

>
> Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
> Signed-off-by: Khem Raj <raj.khem@gmail.com>
> ---
> debuginfod/debuginfod-client.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/debuginfod/debuginfod-client.c
> b/debuginfod/debuginfod-client.c
> index c875ee62..afd223b3 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -231,7 +231,7 @@ debuginfod_config_cache(char *config_path,
> if (fd < 0)
> return -errno;
>  
> - if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> + if (dprintf(fd, "%jd", (intmax_t)cache_config_default_s) < 0)
> return -errno;
> }
>  
> @@ -239,7 +239,7 @@ debuginfod_config_cache(char *config_path,
> FILE *config_file = fopen(config_path, "r");
> if (config_file)
> {
> - if (fscanf(config_file, "%ld", &cache_config) != 1)
> + if (fscanf(config_file, "%jd", (intmax_t*)(&cache_config)) != 1)

This is the wrong fix, a temporary variable should be used. When time_t
is still 32 bits, it means you can accidentally write out of bounds for
times after 2038 on little endian plaforms; and on big endian platforms,
it's always writing out of bounds (and *only out of bounds*, before
2038, so you can't even access the result of fscanf).

Having now taken a second look at the code, I don't think this needs to
be treated as time_t, anyway? cache_config is a `long`, and it's reading
a max timeout value, which is unlikely to go beyond the hundreds of
seconds... Given that the function returns an `int`, I'd even consider
making `cache_config` an `int` directly.

> cache_config = cache_config_default_s;
> fclose(config_file);
> }
> @@ -272,7 +272,7 @@ debuginfod_init_cache (char *cache_path, char
> *interval_path, char *maxage_path)
> if (fd < 0)
> return -errno;
>  
> - if (dprintf(fd, "%ld", cache_clean_default_interval_s) < 0)
> + if (dprintf(fd, "%jd", (intmax_t)cache_clean_default_interval_s) < 0)
> return -errno;
>  
> /* init max age config file. */
> @@ -280,7 +280,7 @@ debuginfod_init_cache (char *cache_path, char
> *interval_path, char *maxage_path)
> && (fd = open(maxage_path, O_CREAT | O_RDWR, DEFFILEMODE)) < 0)
> return -errno;
>  
> - if (dprintf(fd, "%ld", cache_default_max_unused_age_s) < 0)
> + if (dprintf(fd, "%jd", (intmax_t)cache_default_max_unused_age_s) < 0)
> return -errno;
>  
> return 0;
> --
> 2.20.1