uptime -p reports incorrect output after 52 weeks

[Robert Malz]

[ Impact ]

uptime -p will provide incorrect data for 24 hours after exactly 52 weeks. Users and tools utilizing this command will receive example output "up 2 hours" instead of "up 52 weeks, 2 hours". After 24 hours since 52 weeks, uptime -p will report "up 1 year" which is correct output.
Issue is already fixed in upstream https://gitlab.com/procps-ng/procps/-/commit/0496b39876d569fe1cecb76ad5ef212cd14c0374.
Latest procps releases already include this patch (procps 4.0.3 lunar/mantic)
The fix is needed for following set of packages:
procps | 2:3.3.17-6ubuntu2 | jammy
procps | 2:3.3.16-1ubuntu2 | focal

[ Test Plan ]

Reproduction:
UPTIME="31528920 31528800"; mkfifo uptime_fifo; while true; do cat <<<$UPTIME > uptime_fifo; done & sudo mount -obind uptime_fifo /proc/uptime
uptime -p
Running above commands will result in incorrect uptime output.
Testing:
In attached uptime_test_results file there is modified print_uptime function which has been used to test multiple corner cases.

[ Where problems could occur ]

Proposed changes modifies output format of "uptime -p". Issue has been already fixed in latest version of procps package available in lunar/mantic however older releases are based on different code base and patch cannot be directly cherry-picked. Due to backport requirements some code of "uptime" (without -p) has been also changed but this should not impact logic for that usage.
As the change focuses on modifying "uptime -p" output format any potential issues will impact this command.
I have also looked for a reverse dependencies on procps package to check for potential uses of uptime -p, however I was not able to find any. Internally within procps package this functionality is used in "top" application, however for that case "uptime" (without -p) is used.

[ Other Info ]
Bug upstream: https://gitlab.com/procps-ng/procps/-/issues/217
Following patch is needed for older releases: https://gitlab.com/procps-ng/procps/-/commit/0496b39876d569fe1cecb76ad5ef212cd14c0374
Old commit on which upstream patch is based: https://gitlab.com/procps-ng/procps/-/commit/8827c6763f79f77a126968e200b0e402de7cb749
Small change on top of proposed patch (already included in debdiff): https://gitlab.com/procps-ng/procps/-/commit/10824b0655f3eeaeac87ae6e4e3881429a237f3e

[Robert Malz (rmalz)]

debdiff with backport for jammy

[Robert Malz (rmalz)]

debdiff with backport for focal

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "uptime_jammy.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[Paride Legovini (paride)]

Hello, I had a look at the debdiffs and they look good (clean upstream cherry-picks). I think the packages are ready for sponsoring, but before doing so I have a couple of questions.

1. The issue only happens *exactly* after 52 weeks, or something like that, am I right? as I have systems that have been up for more than 1 years and their `uptime -p` is fine:

ubuntu@s1lp04:~$ uptime
 12:33:13 up 498 days, 15:50, 1 user, load average: 0,07, 0,13, 0,40
ubuntu@s1lp04:~$ uptime -p
up 1 year, 19 weeks, 1 day, 15 hours, 50 minutes

2. I don't have a SRU hat to wear, but my impression is that the SRU team won't find that [REGRESSION POTENTIAL] section entirely satisfying. One question to answer is: can any production system be relying on the "wrong" behavior of uptime? What's the worst case scenario caused by the SRU in this case? My impression is that the change is safe, but please expand that section a bit. (Note: the SRU template from https://wiki.ubuntu.com/StableReleaseUpdates now calls that section [Where problems could occur], to make it more clear what it is about.)

[Robert Malz (rmalz)]

Thanks Paride for feedback.
1.
Old calculation is a little bit wrong.
It assumes that we have 52 weeks in the year - which is 60*60*24*7*52 -> 31449600 seconds
(31449600 - 1) will result in 51 weeks, 31449600 will result in 0 weeks (it is assuming year has passed).
However year calculation is different, 60*60*24*365 -> 31536000
(31536000 - 1) will result in 0 years, 31536000 will result in 1 year.

Problem is that between 31449600 and 31536000 uptime -p will return 0 for weeks and 0 for years.
To finish this up, we will have incorrect output for 86400s (24h).

With new patch:
upweeks = (int) uptime_secs / (60*60*24*7);
will return 52 weeks for 31449600->31536000

2. I'll try to add some details from this comment to update [REGRESSION POTENTIAL] section.

[Dan Bungert (dbungert)]

Hi Robert, thanks for the update to the description.

I would like to see two things

1. (Like Paride, I don't have a SRU hat to wear) While this updated description elaborates on what has changed, it doesn't really go into the "what can go wrong" territory. This is an important field for SRU review as we want to consider how problems might manifest, if they are going to.

In this case, we've got an upstream patch that has been around for a while. Have there been further upstream changes in this code area since? You can evaluate that to help establish some history.

We're changing a calculation. The immediate calculation is improved, but what about similar ones? is `uptime -p` correct now for 51 weeks and 53? Similar things like that are good to consider.

2. It would be good to elaborate on the changelog, on what's going on. I think https://www.debian.org/doc/manuals/developers-reference/best-pkging-practices.html#writing-useful-changelog-entries is a good description of what sort of content is desired.

[Robert Malz (rmalz)]

History is a little bit tricky, patch hhttps://gitlab.com/procps-ng/procps/-/commit/8827c6763f79f77a126968e200b0e402de7cb749 (applied in current debdiff) was merged to the master branch but probably was lost while repository was moving to a "newlib".
Last released version https://gitlab.com/procps-ng/procps/-/tags/v3.3.17 did not have this patch applied.
Next release version https://gitlab.com/procps-ng/procps/-/tags/v4.0.0 was based on refactored code.

With newlib as master this commit has been applied with some modifications in https://gitlab.com/procps-ng/procps/-/commit/0496b39876d569fe1cecb76ad5ef212cd14c0374
Looking at that function there is one more change applied to it https://gitlab.com/procps-ng/procps/-/commit/f08082093192b7d93f28517ffc5298172d182a1e which fixes and issue caused by previous modifications.
Differences in both patches are fairly small and affects upminutes/uphours calculation.
However after closer look (and writing some tests to check corner cases) there is one additional bug with updays and uphours calculation in proposed patch.
If there is exactly 60*60*24 (24h) input, updays will be equal to 0 and uphours will be 24%24 -> 0 which is a regression.

Having said that I would not recommend going forward with current proposed patch, another option is to backport https://gitlab.com/procps-ng/procps/-/commit/0496b39876d569fe1cecb76ad5ef212cd14c0374.
However because base code has been modified this patch will not apply on 3.3.16/17 and needs some tweaks.
I already started working on that and output looks promising. There is still 1 issue with exactly 60s of uptime but it comes from the upstream (I have already created an issue in upstream gitlab repo).

Attaching testing results "uptime_test_results".

I'll work on providing new debdiffs and keep improving Bug Description.

[Robert Malz (rmalz)]

Added new debdiffs, updated bug description.

[Andreas Hasenack (ahasenack)]

Time calculations can be so tricky!

The patch is also changing the non-human-readable case (without -p), if I understood it correctly. I think this also needs to be tested.

I think it would be beneficial to include the upcoming fix for the other corner case you found, when uptime is exactly 60s, as that also affects the later versions. That's one less SRU for the future.

Finally, since you have such a nice case, do you think you could add that as a DEP8 test? The bind mount works in a LXD container. The upstream testsuite/uptime.test is too basic to be changed for this case, it's basically just a smoke test.

[Robert Malz (rmalz)]

Hi Andreas,
Regarding output,
non-human-readable output of uptime is not modified.
I had to modify code flow a little bit to not change upminutes and uphours after initial calculation in case of "-p", thus I hid it under if(!human_readable).
Code flow for !human_readable should stay exactly as before.

Regarding 60s fix, it looks like it should be a fairly one line change. I proposed following change
    if (upminutes || (!upminutes && uptime_secs < 60)) { // change < to <=
      pos += sprintf(buf + pos, "%s%d %s", comma > 0 ? ", " : "", upminutes,
                     upminutes != 1 ? "minutes" : "minute");
      comma += 1;
    }
I'll wait for a feedback from upstream

[Steve Langasek (vorlon)]

marking resolved for the devel series, per description.

[Robie Basak (racb)]

Could we have a justification for why this is important for SRU, please? As written, the Impact statement describes a bug, but doesn't present any explanation of why it is important for Ubuntu to fix it in its stable releases. If it is just that the output might be wrong and we'd like it to be correct, then it should be sufficient to be fixed in the next release.

There is a cost to every SRU in terms of review, sponsorship and SRU review time, and then for millions of users to download and install yet another update. I'm sure most users do not want to download many megabytes just for this type of stable release update (this binary is small, but doing this kind of thing across many packages adds up), so there needs to be a strong justification for doing it.

We can stage this change to be bundled with another fix should it ever come, but note that we've been doing this kind of thing for a while and many such changes never land before the release EOLs, meaning that such review efforts were wasted and better spent on other reviews instead.

I'm setting this Won't Fix for now since, as described, the Impact statement does not provide sufficient justification for an SRU.

We can reconsider if we have a report of a real user story that demonstrates why this is of high enough impact to users to be worth fixing it.

[Robie Basak (racb)]

(unsubscribing ~ubuntu-sponsors as given my previous comment there is now nothing remaining to sponsor)