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

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

[Alexander Kanavin]

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.

musl 1.2.0 and above uses time64 on all platforms; 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.

Use a plain int for scanning cache_config, as that's what the function
returns.

Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
 debuginfod/debuginfod-client.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index c875ee62..df9737d2 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -231,15 +231,15 @@ 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;
     }
 
-  long cache_config;
+  int cache_config;
   FILE *config_file = fopen(config_path, "r");
   if (config_file)
     {
-      if (fscanf(config_file, "%ld", &cache_config) != 1)
+      if (fscanf(config_file, "%d", &cache_config) != 1)
         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

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

[Frank Ch. Eigler]

Hi -

> Use intmax_t to print time_t
> [...]
> -      if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> +      if (dprintf(fd, "%jd", (intmax_t)cache_config_default_s) < 0)
> [...]

I'm not a compatibility specialist, but note that elfutils is sometimes
built on decade-old toolchains, where use of %lld and (long long) would
certainly work.  We have such in the code base already, whereas this would
be the first %j.

- FChE

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

[Alexander Kanavin]

According to 'man 3 printf' %j appeared in glibc 2.1 and seems to be C99
feature. That's decidedly ancient.

And may I be allowed to suggest, unless elfutils is tested in upstream CI
with decade-old toolchains, you should not be considering its compatibility
with them.

Alex

On Thu, 25 Nov 2021 at 18:51, Frank Ch. Eigler <fche@redhat.com> wrote:

> Hi -
>
> > Use intmax_t to print time_t
> > [...]
> > -      if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> > +      if (dprintf(fd, "%jd", (intmax_t)cache_config_default_s) < 0)
> > [...]
>
> I'm not a compatibility specialist, but note that elfutils is sometimes
> built on decade-old toolchains, where use of %lld and (long long) would
> certainly work.  We have such in the code base already, whereas this would
> be the first %j.
>
> - FChE
>
>

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

[Frank Ch. Eigler]

Hi -

> According to 'man 3 printf' %j appeared in glibc 2.1 and seems to be C99
> feature. That's decidedly ancient.

OK. 

> And may I be allowed to suggest, unless elfutils is tested in upstream CI
> with decade-old toolchains, you should not be considering its compatibility
> with them.

Deployment of elfutils is not limited to platforms that happen to be
included in upstream CI that you see.  RHEL7 uses glibc 2.17 from
2012; RHEL6 years before that.  We do not wish to unnecessarily break
them.

- FChE

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

[Mark Wielaard]

Hi,

On Sun, Nov 21, 2021 at 11:14:11PM +0100, 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.

OK, so there were some questions about whether using intmax_t and %jd
are portable enough, but I think it is clear that everything these
days support that. So that is good.

> musl 1.2.0 and above uses time64 on all platforms; 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.
> 
> Use a plain int for scanning cache_config, as that's what the function
> returns.

So I think you are correct that printing/scanning/casting the time_t
around is somewhat unfortunate because the size of time_t is not
stable across architectures. Thanks for looking into this. I had no
idea time_t was such a problematic data type.

What makes it even more curious is that debuginfod_config_cache takes
a long, not a time_t, while it is always called with a time_t
argument. And then it returns an int... The result is interpreted as a
negative errno number or is cast back to a time_t if it is positive.

With this patch we write out the interval values "as big as possible"
(intmax_t), but read it back in "as small as possible" (int). Would it
make sense to also try to read it back in as intmax_t and then cast
down to int? Or simply always cast down to int before writing it out,
so reading and writing use the same data type?

We seem to not expect these intervals to be much bigger than a week,
so an int should always be big enough (even when stretched up to a
whole year).

Thanks,

Mark

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

[Alexander Kanavin]

I'm not sure; the point of this patch is simply to ensure debuginfod builds
everywhere without errors. Making the types consistent is perhaps better
done as a followup?

Alex

On Sun, 5 Dec 2021 at 21:31, Mark Wielaard <mark@klomp.org> wrote:

> Hi,
>
> On Sun, Nov 21, 2021 at 11:14:11PM +0100, 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.
>
> OK, so there were some questions about whether using intmax_t and %jd
> are portable enough, but I think it is clear that everything these
> days support that. So that is good.
>
> > musl 1.2.0 and above uses time64 on all platforms; 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.
> >
> > Use a plain int for scanning cache_config, as that's what the function
> > returns.
>
> So I think you are correct that printing/scanning/casting the time_t
> around is somewhat unfortunate because the size of time_t is not
> stable across architectures. Thanks for looking into this. I had no
> idea time_t was such a problematic data type.
>
> What makes it even more curious is that debuginfod_config_cache takes
> a long, not a time_t, while it is always called with a time_t
> argument. And then it returns an int... The result is interpreted as a
> negative errno number or is cast back to a time_t if it is positive.
>
> With this patch we write out the interval values "as big as possible"
> (intmax_t), but read it back in "as small as possible" (int). Would it
> make sense to also try to read it back in as intmax_t and then cast
> down to int? Or simply always cast down to int before writing it out,
> so reading and writing use the same data type?
>
> We seem to not expect these intervals to be much bigger than a week,
> so an int should always be big enough (even when stretched up to a
> whole year).
>
> Thanks,
>
> Mark
>

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

[Mark Wielaard]

Hi Alexander,

On Sun, 2021-12-05 at 21:45 +0100, Alexander Kanavin wrote:
> I'm not sure; the point of this patch is simply to ensure debuginfod builds
> everywhere without errors. Making the types consistent is perhaps better
> done as a followup?

I think the issue of the code not compiling in some environments is
because of the inconsistent use of types. So I rather fix the build
errors by fixing the used types than to simply cast the errors away.

Cheers,

Mark

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

[Alexander Kanavin]

Alright, I just sent a patch that replaces time_t with long instead. I
tested it on x86, x86-x32, x86-64.

Alex

On Wed, 8 Dec 2021 at 16:31, Mark Wielaard <mark@klomp.org> wrote:

> Hi Alexander,
>
> On Sun, 2021-12-05 at 21:45 +0100, Alexander Kanavin wrote:
> > I'm not sure; the point of this patch is simply to ensure debuginfod
> builds
> > everywhere without errors. Making the types consistent is perhaps better
> > done as a followup?
>
> I think the issue of the code not compiling in some environments is
> because of the inconsistent use of types. So I rather fix the build
> errors by fixing the used types than to simply cast the errors away.
>
> Cheers,
>
> Mark
>

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

[Frank Ch. Eigler]

Hi -

> [...]
> We seem to not expect these intervals to be much bigger than a week,
> so an int should always be big enough (even when stretched up to a
> whole year).

Yes, ints are fine for these humane-number-of-seconds kinds of values
in the cache configuration.  There's no need for maximum length
integers or even longs for purposes of storage/parse.  In the later
interval arithmetic related to time_t values, we can widen them to
(time_t) then and there.

- FChE

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

[Alexander Kanavin]

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

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

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

[Érico Nogueira]

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

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

[Alexander Kanavin]

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

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

[Khem Raj]

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