[nft PATCH 1/2] meta: don't assume time_t is 64 bit in date_type_print()

[nft PATCH 1/2] meta: don't assume time_t is 64 bit in date_type_print()

[Thomas Haller]

time_t on 32bit arch is not uint64_t. Even if it always were, it would
be ugly to make such an assumption (without a static assert). Copy the
value to a time_t variable first.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/meta.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/meta.c b/src/meta.c
index 822c2fd12b6f..0d4ae0261ff2 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -385,20 +385,23 @@ const struct datatype ifname_type = {
 
 static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 {
-	uint64_t tstamp = mpz_get_uint64(expr->value);
+	uint64_t tstamp64 = mpz_get_uint64(expr->value);
+	time_t tstamp;
 	struct tm *tm, *cur_tm;
 	char timestr[21];
 
 	/* Convert from nanoseconds to seconds */
-	tstamp /= 1000000000L;
+	tstamp64 /= 1000000000L;
 
 	/* Obtain current tm, to add tm_gmtoff to the timestamp */
-	cur_tm = localtime((time_t *) &tstamp);
+	tstamp = tstamp64;
+	cur_tm = localtime(&tstamp);
 
 	if (cur_tm)
-		tstamp += cur_tm->tm_gmtoff;
+		tstamp64 += cur_tm->tm_gmtoff;
 
-	if ((tm = gmtime((time_t *) &tstamp)) != NULL &&
+	tstamp = tstamp64;
+	if ((tm = gmtime(&tstamp)) != NULL &&
 	     strftime(timestr, sizeof(timestr) - 1, "%Y-%m-%d %T", tm))
 		nft_print(octx, "\"%s\"", timestr);
 	else
-- 
2.41.0

[nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Thomas Haller]

These functions are POSIX.1-2001. We should have them in all
environments we care about.

Use them as they are thread-safe.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/meta.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/meta.c b/src/meta.c
index 0d4ae0261ff2..d087e56e1b84 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -387,7 +387,7 @@ static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 {
 	uint64_t tstamp64 = mpz_get_uint64(expr->value);
 	time_t tstamp;
-	struct tm *tm, *cur_tm;
+	struct tm tm;
 	char timestr[21];
 
 	/* Convert from nanoseconds to seconds */
@@ -395,14 +395,12 @@ static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 
 	/* Obtain current tm, to add tm_gmtoff to the timestamp */
 	tstamp = tstamp64;
-	cur_tm = localtime(&tstamp);
-
-	if (cur_tm)
-		tstamp64 += cur_tm->tm_gmtoff;
+	if (localtime_r(&tstamp, &tm))
+		tstamp64 += tm.tm_gmtoff;
 
 	tstamp = tstamp64;
-	if ((tm = gmtime(&tstamp)) != NULL &&
-	     strftime(timestr, sizeof(timestr) - 1, "%Y-%m-%d %T", tm))
+	if (gmtime_r(&tstamp, &tm) &&
+	     strftime(timestr, sizeof(timestr) - 1, "%Y-%m-%d %T", &tm))
 		nft_print(octx, "\"%s\"", timestr);
 	else
 		nft_print(octx, "Error converting timestamp to printed time");
@@ -410,7 +408,8 @@ static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 
 static bool parse_iso_date(uint64_t *tstamp, const char *sym)
 {
-	struct tm tm, *cur_tm;
+	struct tm cur_tm;
+	struct tm tm;
 	time_t ts;
 
 	memset(&tm, 0, sizeof(struct tm));
@@ -432,14 +431,15 @@ success:
 	 */
 	ts = timegm(&tm);
 
-	/* Obtain current tm as well (at the specified time), so that we can substract tm_gmtoff */
-	cur_tm = localtime(&ts);
+	if (ts == (time_t) -1)
+		return false;
 
-	if (ts == (time_t) -1 || cur_tm == NULL)
+	/* Obtain current tm as well (at the specified time), so that we can substract tm_gmtoff */
+	if (!localtime_r(&ts, &cur_tm))
 		return false;
 
 	/* Substract tm_gmtoff to get the current time */
-	*tstamp = ts - cur_tm->tm_gmtoff;
+	*tstamp = ts - cur_tm.tm_gmtoff;
 
 	return true;
 }
@@ -494,15 +494,13 @@ static void day_type_print(const struct expr *expr, struct output_ctx *octx)
 static void hour_type_print(const struct expr *expr, struct output_ctx *octx)
 {
 	uint32_t seconds = mpz_get_uint32(expr->value), minutes, hours;
-	struct tm *cur_tm;
+	struct tm cur_tm;
 	time_t ts;
 
 	/* Obtain current tm, so that we can add tm_gmtoff */
 	ts = time(NULL);
-	cur_tm = localtime(&ts);
-
-	if (cur_tm)
-		seconds = (seconds + cur_tm->tm_gmtoff) % SECONDS_PER_DAY;
+	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm))
+		seconds = (seconds + cur_tm.tm_gmtoff) % SECONDS_PER_DAY;
 
 	minutes = seconds / 60;
 	seconds %= 60;
@@ -520,7 +518,9 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
 					    struct expr **res)
 {
 	struct error_record *er;
-	struct tm tm, *cur_tm;
+	struct tm cur_tm_data;
+	struct tm *cur_tm;
+	struct tm tm;
 	uint32_t result;
 	uint64_t tmp;
 	char *endptr;
@@ -537,7 +537,10 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
 
 	/* Obtain current tm, so that we can substract tm_gmtoff */
 	ts = time(NULL);
-	cur_tm = localtime(&ts);
+	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm_data))
+		cur_tm = &cur_tm_data;
+	else
+		cur_tm = NULL;
 
 	endptr = strptime(sym->identifier, "%T", &tm);
 	if (endptr && *endptr == '\0')
-- 
2.41.0

Re: [nft PATCH 1/2] meta: don't assume time_t is 64 bit in date_type_print()

[Pablo Neira Ayuso]

On Tue, Aug 22, 2023 at 10:13:09AM +0200, Thomas Haller wrote:
> time_t on 32bit arch is not uint64_t. Even if it always were, it would
> be ugly to make such an assumption (without a static assert). Copy the
> value to a time_t variable first.

Applied, thanks.

Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Pablo Neira Ayuso]

On Tue, Aug 22, 2023 at 10:13:10AM +0200, Thomas Haller wrote:
> These functions are POSIX.1-2001. We should have them in all
> environments we care about.
> 
> Use them as they are thread-safe.

Also applied, thanks

Re: [nft PATCH 1/2] meta: don't assume time_t is 64 bit in date_type_print()

[Pablo Neira Ayuso]

On Tue, Aug 22, 2023 at 10:13:09AM +0200, Thomas Haller wrote:
> diff --git a/src/meta.c b/src/meta.c
> index 822c2fd12b6f..0d4ae0261ff2 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -385,20 +385,23 @@ const struct datatype ifname_type = {
>  
>  static void date_type_print(const struct expr *expr, struct output_ctx *octx)
>  {
> -	uint64_t tstamp = mpz_get_uint64(expr->value);
> +	uint64_t tstamp64 = mpz_get_uint64(expr->value);
> +	time_t tstamp;
>  	struct tm *tm, *cur_tm;
>  	char timestr[21];

For the record: I made this edit before applying:

	uint64_t tstamp64 = mpz_get_uint64(expr->value);
  	struct tm *tm, *cur_tm;
  	char timestr[21];
	time_t tstamp;

following reverse xmas tree layout, it is a comestic coding style
issue.

Not all the codebase follows this approach, but this is usually
preferred.

Thanks.

Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Thomas Haller]

On Tue, 2023-08-22 at 10:54 +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 22, 2023 at 10:13:10AM +0200, Thomas Haller wrote:
> > These functions are POSIX.1-2001. We should have them in all
> > environments we care about.
> > 
> > Use them as they are thread-safe.
> 
> Also applied, thanks
> 


Hi Pablo,



One more consideration, that I didn't realize before. Sorry about that.


localtime() will always call tzset(). And localtime_r() is documented
that it may not call it.

  https://linux.die.net/man/3/localtime_r


I checked implementations, AFAIS, musl will always call do_tzset()
([1]). glibc will only ensure that tzset() was called at least once
([2]).

[1] https://git.musl-libc.org/cgit/musl/tree/src/time/__tz.c?id=83b858f83b658bd34eca5d8ad4d145f673ae7e5e#n369
[2] https://codebrowser.dev/glibc/glibc/time/tzset.c.html#577


It's not clear to me, whether it would be more correct/desirable to
always call tzset() before localtime_r(). I think it would only matter,
if the timezone were to change (e.g. update /etc/localtime).

nftables calls localtime_r() from print/parse functions. Presumably, we
will print/parse several timestamps during a larger operation, it would
be odd to change/reload the timezone in between or to meaningfully
support that.


I think it is all good, nothing to change. Just to be aware of.


Thomas

Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Thomas Haller]

On Tue, 2023-08-22 at 13:39 +0200, Thomas Haller wrote:
> On Tue, 2023-08-22 at 10:54 +0200, Pablo Neira Ayuso wrote:
> 
> 
> nftables calls localtime_r() from print/parse functions. Presumably,
> we
> will print/parse several timestamps during a larger operation, it
> would
> be odd to change/reload the timezone in between or to meaningfully
> support that.
> 
> 
> I think it is all good, nothing to change. Just to be aware of.
> 

Thinking some more, the "problem" is that when we parse a larger data,
then multiple subfields are parsed. Thereby we call "time()" and
"localtime()" multiple times. The time() keeps ticking, and time and tz
can be reset at any moment -- so we see different time/tz, in the
middle of parsing the larger set of data.

What IMO should happen, is that for one parse operation, we call such
operations at most once, and cache them in `struct netlink_parse_ctx`.


Is that considered a problem to be solved? Seems simple. Would you
accept a patch for that?


Thomas

Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Pablo Neira Ayuso]

On Tue, Aug 22, 2023 at 01:39:20PM +0200, Thomas Haller wrote:
> Hi Pablo,
> 
> One more consideration, that I didn't realize before. Sorry about that.
> 
> localtime() will always call tzset(). And localtime_r() is documented
> that it may not call it.
> 
>   https://linux.die.net/man/3/localtime_r
> 
> I checked implementations, AFAIS, musl will always call do_tzset()
> ([1]). glibc will only ensure that tzset() was called at least once
> ([2]).
> 
> [1] https://git.musl-libc.org/cgit/musl/tree/src/time/__tz.c?id=83b858f83b658bd34eca5d8ad4d145f673ae7e5e#n369
> [2] https://codebrowser.dev/glibc/glibc/time/tzset.c.html#577
> 
> It's not clear to me, whether it would be more correct/desirable to
> always call tzset() before localtime_r(). I think it would only matter,
> if the timezone were to change (e.g. update /etc/localtime).
>
> nftables calls localtime_r() from print/parse functions. Presumably, we
> will print/parse several timestamps during a larger operation, it would
> be odd to change/reload the timezone in between or to meaningfully
> support that.

You mean, timezone change while there is a 'list ruleset' command
might be an issue is what you mean?

> I think it is all good, nothing to change. Just to be aware of.

Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions

[Pablo Neira Ayuso]

On Tue, Aug 22, 2023 at 05:15:14PM +0200, Thomas Haller wrote:
> On Tue, 2023-08-22 at 13:39 +0200, Thomas Haller wrote:
> > On Tue, 2023-08-22 at 10:54 +0200, Pablo Neira Ayuso wrote:
> > 
> > 
> > nftables calls localtime_r() from print/parse functions. Presumably,
> > we
> > will print/parse several timestamps during a larger operation, it
> > would
> > be odd to change/reload the timezone in between or to meaningfully
> > support that.
> > 
> > 
> > I think it is all good, nothing to change. Just to be aware of.
> > 
> 
> Thinking some more, the "problem" is that when we parse a larger data,
> then multiple subfields are parsed. Thereby we call "time()" and
> "localtime()" multiple times. The time() keeps ticking, and time and tz
> can be reset at any moment -- so we see different time/tz, in the
> middle of parsing the larger set of data.
> 
> What IMO should happen, is that for one parse operation, we call such
> operations at most once, and cache them in `struct netlink_parse_ctx`.
>
> Is that considered a problem to be solved? Seems simple. Would you
> accept a patch for that?

Caching this information in the context should be fine, and it might
speed up things for a large batch? How complicate will the update look
like?