shadow-maint / shadow

Upstream shadow tree
Other
292 stars 228 forks source link

Chage miscalculation #939

Closed jubalh closed 6 months ago

jubalh commented 7 months ago

We noticed something strange on our openSUSE systems. Unfortunately I didn't yet have the time to look into it. Maybe it's best that I open this upstream bug in case it's a wider problem which we might want to fix before the next stable release.

# date
Tue Sep  1 14:16:35 CEST 2020
# chage -d today joesix
# chage -l joesix
Last password change                                    : Sep 02, 2020
[...]
# chage -d yesterday joesix
# chage -l joesix
Last password change                                    : Sep 02, 2020
[...]
# chage -d "2 days ago" joesix
# chage -l joesix
Last password change                                    : Aug 31, 2020
[...]
# date
Wed Sep  2 13:57:31 CEST 2020

# chage -l joesix
Last password change                    : Sep 02, 2020

# date
Wed Sep  2 14:05:21 CEST 2020

# chage -d 'today' joesix
# chage -l joesix
Last password change                    : Sep 03, 2020

Maybe rollover is at 12:00:00 UTC?

alejandro-colomar commented 7 months ago

This code seems to match the 12:00 UTC rollover:

$ grepc strtoday . | tail
        return retdate;
    }

    t = get_date (str, NULL);
    if ((time_t) - 1 == t) {
        return -2;
    }
    /* convert seconds to days since 1970-01-01 */
    return (t + DAY / 2) / DAY;
}

I don't get why it adds that half day. :|

alx@debian:~/src/shadow/shadow/ts$ git blame HEAD -- lib/strtoday.c | grep DAY
89a7ee7b2 libmisc/strtoday.c (Iker Pedrosa      2023-06-07 14:58:34 +0200 76)   return (t + DAY / 2) / DAY;
alx@debian:~/src/shadow/shadow/ts$ git show 89a7ee7b2 -- libmisc/strtoday.c | grep DAY
-   return (long) (t + DAY / 2) / DAY;
+   return (t + DAY / 2) / DAY;
alx@debian:~/src/shadow/shadow/ts$ git blame 89a7ee7b2^ -- libmisc/strtoday.c | grep DAY
815ffb7d3 (nekral-guest 2008-06-13 19:48:11 +0000 76)   return (long) (t + DAY / 2) / DAY;
alx@debian:~/src/shadow/shadow/ts$ git show 815ffb7d3 -- libmisc/strtoday.c | grep DAY
-   return (t + DAY / 2) / DAY;
+   return (long) (t + DAY / 2) / DAY;
-       return result / DAY;    /* success */
+       return (long) (result / DAY);   /* success */
alx@debian:~/src/shadow/shadow/ts$ git blame 815ffb7d3^ -- libmisc/strtoday.c | grep DAY
effd479bf (nekral-guest 2007-10-07 11:45:23 +0000  79)  return (t + DAY / 2) / DAY;
effd479bf (nekral-guest 2007-10-07 11:45:23 +0000 139)      return result / DAY;    /* success */
alx@debian:~/src/shadow/shadow/ts$ git show effd479bf -- libmisc/strtoday.c | grep DAY
-   return (t + DAY/2)/DAY;
+   return (t + DAY / 2) / DAY;
-       return result / DAY;  /* success */
+       return result / DAY;    /* success */
alx@debian:~/src/shadow/shadow/ts$ git blame effd479bf^ -- libmisc/strtoday.c | grep DAY
45c6603cc (nekral-guest 2007-10-07 11:44:02 +0000  80)  return (t + DAY/2)/DAY;
45c6603cc (nekral-guest 2007-10-07 11:44:02 +0000 142)      return result / DAY;  /* success */
alx@debian:~/src/shadow/shadow/ts$ git show 45c6603cc -- libmisc/strtoday.c | grep DAY
+   return (t + DAY/2)/DAY;
+       return result / DAY;  /* success */
alx@debian:~/src/shadow/shadow/ts$ git log -1 45c6603cc | head
commit 45c6603cc86c5881b00ac40e0f9fe548c30ff6be
Author: nekral-guest <nekral-guest@5a98b0ae-9ef6-0310-add3-de5d479b70d7>
Date:   Sun Oct 7 11:44:02 2007 +0000

    [svn-upgrade] Integrating new upstream version, shadow (19990709)
alejandro-colomar commented 7 months ago

In the long term, I think we should move away from lib/getdate.y. I suspect it was written in a time when getdate(3) might not be portable enough.

To-day, using POSIX.1-2008, we can just day = mktime(getdate(string)) / DAY; and call it a day. And we would benefit of any bug fixes that libc applies to those functions, instead of the monster we have in lib/getdate.y, which I don't think is maintainable.

ikerexxe commented 7 months ago

In the long term, I think we should move away from lib/getdate.y. I suspect it was written in a time when getdate(3) might not be portable enough.

To-day, using POSIX.1-2008, we can just day = mktime(getdate(string)) / DAY; and call it a day. And we would benefit of any bug fixes that libc applies to those functions, instead of the monster we have in lib/getdate.y, which I don't think is maintainable.

I hope you can hear me clapping my hands! Yes, let's move away from getdate.y to a more standard solution.

hallyn commented 6 months ago

@jubalh could you verify whether master now fixes these?

alejandro-colomar commented 6 months ago

@jubalh could you verify whether master now fixes these?

The patch we merged recently only fixed the second part of the report, but this is really two bugs, and one is still there. The fix for the first part of the report is

(although it's YACC code, so I have no idea if I'm breaking something else with my patch; please test)

See also:

alejandro-colomar commented 6 months ago

In the long term, I think we should move away from lib/getdate.y. I suspect it was written in a time when getdate(3) might not be portable enough.

To-day, using POSIX.1-2008, we can just day = mktime(getdate(string)) / DAY; and call it a day. And we would benefit of any bug fixes that libc applies to those functions, instead of the monster we have in lib/getdate.y, which I don't think is maintainable.

I'll call the time doctor. :)

Hi @eggert!

We have some very old mktime(getdate())-like implementation in shadow, called get_date(), written in YACC, and it's buggy (https://github.com/shadow-maint/shadow/pull/952).

I'd like to replace it by something that is easier to maintain, if possible. But also, I'd like to not regress on what it accepts (unless we need for major reasons).

From what I can see, getdate(3), being based on strptime(3), can't interpret things like "yesterday", which our get_date() accepts.

Do you know of any library function that does this? Or failing that, is there any standard or common system DATEMSK file so that we can use it without having to define it ourselves?

Have a lovely day! Alex

eggert commented 6 months ago

On 2024-02-16 02:53, Alejandro Colomar wrote:

Do you know of any library function that does this? Or failing that, is there any standard or common system DATEMSK file so that we can use it without having to define it ourselves?

Gnulib has a parse_datetime module, which is what GNU 'date' uses. So if you want to be compatible with GNU 'date' that's the way to go. It supports strings like "yesterday".

No date parser is perfect. They all have glitches.

alejandro-colomar commented 6 months ago

Hi Paul,

On 2024-02-16 02:53, Alejandro Colomar wrote:

Do you know of any library function that does this? Or failing that, is there any standard or common system DATEMSK file so that we can use it without having to define it ourselves?

Gnulib has a parse_datetime module, which is what GNU 'date' uses. So if you want to be compatible with GNU 'date' that's the way to go. It supports strings like "yesterday". No date parser is perfect. They all have glitches.

Thanks. I was worried that your answer would probably be Gnulib. :p

Since Gnulib is (L)GPL'd and this project is BSD'd, and it's a source-code library, I guess they are incompatible. (And I would prefer a dependency that is an actual system library.)

Hmmm, I guess we need to stick to this yacc file.

Have a lovely night! Alex