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

  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

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(-)
  

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
  

On Sat, 20 Nov 2021 at 05:13, Érico Nogueira <ericonr@disroot.org> wrote:

> 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.
>

My original version did use %lld, but Khem tweaked it to use intmax_t.
Perhaps he can comment?


> 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.
>

Yes, I can add that.


> 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.
>

I can fix this as well if Khem confirms.

Thanks for the review.

Alex
  

On Sat, Nov 20, 2021 at 12:11 AM Alexander Kanavin
<alex.kanavin@gmail.com> wrote:
>
> On Sat, 20 Nov 2021 at 05:13, Érico Nogueira <ericonr@disroot.org> wrote:
>>
>> 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.
>
>
> My original version did use %lld, but Khem tweaked it to use intmax_t. Perhaps he can comment?

intmax_t is not particularly bounded like long long int and sounded
the right choice here say if we have 128bit
machines in future.

>
>>
>> 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.
>
>
> Yes, I can add that.
>
>>
>> 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).

yes it seems that way, thanks for catching it.`

>>
>> 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.
>
>
> I can fix this as well if Khem confirms.

treating int is fine. debuginfod_config_cache() is still passed time_t as second
param in few call sites which are assigned to cache_config here, so we will
still need to convert/truncate that assignment.

>
> Thanks for the review.
>
> Alex