Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Half-life #942

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Half-life #942

merged 1 commit into from
Feb 13, 2024

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Feb 8, 2024

@jubalh Would you please test this (I didn't test it at all yet)? Thanks!

@alejandro-colomar
Copy link
Collaborator Author

v2 changes:

$ git range-diff shadow/master 3d4a3a3 HEAD
1:  d636dbc7 < -:  -------- src/login.c: Fix off-by-one buggs
2:  855e3f2e < -:  -------- lib/: Don't say 'len' where 'size' is meant
3:  d893a0d6 < -:  -------- src/login.c: Fix off-by-one bugss
4:  12fd1121 < -:  -------- lib/chkname.c: is_valid_user_name(): Remove unnecessary check
5:  e5ce948a < -:  -------- lib/chkname.c: is_valid_user_name(): Avoid a cast
6:  3d4a3a39 = 1:  1b4652b0 lib/strtoday.c: strtoday(): Fix calculation

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 8, 2024

v2b changes:

  • Capitalize Epoch
$ git range-diff shadow/master gh/half-life half-life 
1:  1b4652b0 ! 1:  e5d83e68 lib/strtoday.c: strtoday(): Fix calculation
    @@ Commit message
     
         Also, remove the comment.  It's likely to get stale.
     
    -    So, get_date() gets the number of seconds since the epoch.  I wonder how
    +    So, get_date() gets the number of seconds since the Epoch.  I wonder how
         that thing works, but I'll assume it's something similar to strptime(3)
         + mktime(3).  After that, we need to convert seconds since Epoch to days
         since Epoch.  That should be a simple division, AFAICS, since Epoch is

@alejandro-colomar alejandro-colomar linked an issue Feb 8, 2024 that may be closed by this pull request
@alejandro-colomar alejandro-colomar marked this pull request as draft February 8, 2024 12:11
@alejandro-colomar alejandro-colomar changed the title Half life Half-life Feb 8, 2024
@alejandro-colomar
Copy link
Collaborator Author

v2c changes:

  • Refer to getdate(3), not strptime(3), in commit message.
$ git range-diff shadow/master gh/half-life half-life 
1:  e5d83e68 ! 1:  1b054708 lib/strtoday.c: strtoday(): Fix calculation
    @@ Commit message
         Also, remove the comment.  It's likely to get stale.
     
         So, get_date() gets the number of seconds since the Epoch.  I wonder how
    -    that thing works, but I'll assume it's something similar to strptime(3)
    +    that thing works, but I'll assume it's something similar to getdate(3)
         + mktime(3).  After that, we need to convert seconds since Epoch to days
         since Epoch.  That should be a simple division, AFAICS, since Epoch is
         "1970‐01‐01 00:00:00 +0000 (UTC)".  See mktime(3).

@kenion
Copy link

kenion commented Feb 13, 2024

Hi @alejandro-colomar, I'm helping @jubalh out with testing for this. I pulled this patch in and am still seeing the date discrepancy (see screenshot)
screenshot
Testing occurred on 2024-02-12 in CET time zone. With the patch in place, chage still behaves differently for 'yesterday' (incorrect) vs '1 day ago' (correct).

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 13, 2024

Hi @kenion ,

Hi @alejandro-colomar, I'm helping @jubalh out with testing for this. I pulled this patch in and am still seeing the date discrepancy (see screenshot) screenshot Testing occurred on 2024-02-12 in CET time zone. With the patch in place, chage still behaves differently for 'yesterday' (incorrect) vs '1 day ago' (correct).

Thanks for testing!

Does it at least fix the second issue (the wrap at 12:00 UTC, 14:00 CET)? Maybe we're seeing two different bugs.

Have a lovely day!
Alex

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 13, 2024

I expect that this shouldn't be reproducible anymore:

# 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

#939 (comment)

@kenion
Copy link

kenion commented Feb 13, 2024

Thanks @alejandro-colomar, you're right. I can no longer replicate the timezone wrap.

Days officially roll over at 00:00 UTC, not at 12:00 UTC.  I see no
reason to add that half day.

Also, remove the comment.  It's likely to get stale.

So, get_date() gets the number of seconds since the Epoch.  I wonder how
that thing works, but I'll assume it's something similar to getdate(3)
+ mktime(3).  After that, we need to convert seconds since Epoch to days
since Epoch.  That should be a simple division, AFAICS, since Epoch is
"1970‐01‐01 00:00:00 +0000 (UTC)".  See mktime(3).

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Link: <shadow-maint#939>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Tested-by: Gus Kenion <https://github.com/kenion>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator Author

v2d changes:

  • Tested-by @kenion
  • Doesn't close the issue entirely.
$ git range-diff shadow/master gh/half-life half-life 
1:  1b054708 ! 1:  ee1e9ae3 lib/strtoday.c: strtoday(): Fix calculation
    @@ Commit message
         "1970‐01‐01 00:00:00 +0000 (UTC)".  See mktime(3).
     
         Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
    -    Closes: <https://github.com/shadow-maint/shadow/issues/939>
    +    Link: <https://github.com/shadow-maint/shadow/issues/939>
         Reported-by: Michael Vetter <jubalh@iodoru.org>
    +    Tested-by: Gus Kenion <https://github.com/kenion>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/strtoday.c ##

@alejandro-colomar alejandro-colomar marked this pull request as ready for review February 13, 2024 16:13
@hallyn hallyn merged commit 1175932 into shadow-maint:master Feb 13, 2024
9 checks passed
alejandro-colomar added a commit that referenced this pull request Feb 14, 2024
Days officially roll over at 00:00 UTC, not at 12:00 UTC.  I see no
reason to add that half day.

Also, remove the comment.  It's likely to get stale.

So, get_date() gets the number of seconds since the Epoch.  I wonder how
that thing works, but I'll assume it's something similar to getdate(3)
+ mktime(3).  After that, we need to convert seconds since Epoch to days
since Epoch.  That should be a simple division, AFAICS, since Epoch is
"1970‐01‐01 00:00:00 +0000 (UTC)".  See mktime(3).

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Link: <#939>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Tested-by: Gus Kenion <https://github.com/kenion>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 1175932 ("lib/strtoday.c: strtoday(): Fix calculation")
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Link: <#942>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar alejandro-colomar deleted the half-life branch February 15, 2024 11:15
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 15, 2024
Instead of adding 1, we should add the value the we stored previously in
the variable.

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Closes: <shadow-maint#939>
Link: <shadow-maint#942>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This was referenced Feb 15, 2024
Merged
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 15, 2024
Instead of adding 1, we should add the value the we stored previously in
the variable.

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Closes: <shadow-maint#939>
Link: <shadow-maint#942>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
hallyn pushed a commit that referenced this pull request Feb 17, 2024
Instead of adding 1, we should add the value the we stored previously in
the variable.

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Closes: <#939>
Link: <#942>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit that referenced this pull request Feb 17, 2024
Instead of adding 1, we should add the value the we stored previously in
the variable.

Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Closes: <#939>
Link: <#942>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 4d139ca ("lib/getdate.y: get_date(): Fix calculation")
Link: <#952>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 14, 2025
This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since
Epoch.  We don't have enough precision to translate it into local time.
Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since
printing a date without time-zone information means that the date is a
local date, but what we're printing is a UTC date.  A future patch
should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: <https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20>
Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430>
Link: <https://lists.iana.org/hyperkitty/list/tz@iana.org/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/>
Link: <shadow-maint#1202>
Link: <shadow-maint#1057>
Link: <shadow-maint#939>
Link: <shadow-maint#1058>
Link: <shadow-maint#1059 (comment)>
Link: <shadow-maint#952>
Link: <shadow-maint#942>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Reported-by: Alejandro Colomar <alx@kernel.org>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Lee Garrett <lgarrett@rocketjump.eu>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Tim Parenti <tim@timtimeonline.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Brian Inglis <Brian.Inglis@SystematicSW.ab.ca>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 16, 2025
This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since
Epoch.  We don't have enough precision to translate it into local time.
Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since
printing a date without time-zone information means that the date is a
local date, but what we're printing is a UTC date.  A future patch
should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: <https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20>
Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430>
Link: <https://lists.iana.org/hyperkitty/list/tz@iana.org/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/>
Link: <shadow-maint#1202>
Link: <shadow-maint#1057>
Link: <shadow-maint#939>
Link: <shadow-maint#1058>
Link: <shadow-maint#1059 (comment)>
Link: <shadow-maint#952>
Link: <shadow-maint#942>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Reported-by: Alejandro Colomar <alx@kernel.org>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Lee Garrett <lgarrett@rocketjump.eu>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Tim Parenti <tim@timtimeonline.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Brian Inglis <Brian.Inglis@SystematicSW.ab.ca>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 25, 2025
This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since
Epoch.  We don't have enough precision to translate it into local time.
Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since
printing a date without time-zone information means that the date is a
local date, but what we're printing is a UTC date.  A future patch
should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: <https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20>
Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430>
Link: <https://lists.iana.org/hyperkitty/list/tz@iana.org/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/>
Link: <shadow-maint#1202>
Link: <shadow-maint#1057>
Link: <shadow-maint#939>
Link: <shadow-maint#1058>
Link: <shadow-maint#1059 (comment)>
Link: <shadow-maint#952>
Link: <shadow-maint#942>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Reported-by: Alejandro Colomar <alx@kernel.org>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Lee Garrett <lgarrett@rocketjump.eu>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Tim Parenti <tim@timtimeonline.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Brian Inglis <Brian.Inglis@SystematicSW.ab.ca>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit that referenced this pull request Feb 25, 2025
This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since
Epoch.  We don't have enough precision to translate it into local time.
Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since
printing a date without time-zone information means that the date is a
local date, but what we're printing is a UTC date.  A future patch
should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: <https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20>
Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430>
Link: <https://lists.iana.org/hyperkitty/list/tz@iana.org/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/>
Link: <#1202>
Link: <#1057>
Link: <#939>
Link: <#1058>
Link: <#1059 (comment)>
Link: <#952>
Link: <#942>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Reported-by: Gus Kenion <https://github.com/kenion>
Reported-by: Alejandro Colomar <alx@kernel.org>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Reported-by: Lee Garrett <lgarrett@rocketjump.eu>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Tim Parenti <tim@timtimeonline.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Brian Inglis <Brian.Inglis@SystematicSW.ab.ca>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants