[PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
From: Alexander Kanavin
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
Signed-off-by: Khem Raj
---
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
On Sat, 20 Nov 2021 at 05:13, Érico Nogueira 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
[PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
From: Alexander Kanavin
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
Signed-off-by: Khem Raj
---
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
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 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
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 wrote: > Hi, > > On Sun, Nov 21, 2021 at 11:14:11PM +0100, Alexander Kanavin via > Elfutils-devel wrote: > > From: Alexander Kanavin > > > > 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 >
[PATCH] debuginfod/debuginfod-client.c: use long for cache time configurations
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 a plain long everywhere as the intervals are small enough that it will not be problematic. Signed-off-by: Alexander Kanavin --- debuginfod/debuginfod-client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c index 9bf97bfc..024b0954 100644 --- a/debuginfod/debuginfod-client.c +++ b/debuginfod/debuginfod-client.c @@ -135,17 +135,17 @@ struct debuginfod_client how frequently the cache should be cleaned. The file's st_mtime represents the time of last cleaning. */ static const char *cache_clean_interval_filename = "cache_clean_interval_s"; -static const time_t cache_clean_default_interval_s = 86400; /* 1 day */ +static const long cache_clean_default_interval_s = 86400; /* 1 day */ /* The cache_miss_default_s within the debuginfod cache specifies how frequently the 000-permision file should be released.*/ -static const time_t cache_miss_default_s = 600; /* 10 min */ +static const long cache_miss_default_s = 600; /* 10 min */ static const char *cache_miss_filename = "cache_miss_s"; /* The cache_max_unused_age_s file within the debuginfod cache specifies the the maximum time since last access that a file will remain in the cache. */ static const char *cache_max_unused_age_filename = "max_unused_age_s"; -static const time_t cache_default_max_unused_age_s = 604800; /* 1 week */ +static const long cache_default_max_unused_age_s = 604800; /* 1 week */ /* Location of the cache of files downloaded from debuginfods. The default parent directory is $HOME, or '/' if $HOME doesn't exist. */ -- 2.20.1
Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t
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 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 >
