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

Fix possible integer overflow in parse_date_time #2687

Closed
wants to merge 1 commit into from

Conversation

a3a3el
Copy link
Contributor

@a3a3el a3a3el commented Apr 12, 2024

g_date_time_to_unix returns a gint64. Since 27c0728 ("utils: replace time_to_string with fmt-based formatting"), parse_date_time has returned a time_t, which has historically been a 32-bit type. There is, therefore, the possibility of integer over- and underflow. Define TIME_MIN and TIME_MAX based on the size of time_t and clamp the return value of parse_date_time between these.

I'm more a C than a C++ programmer, so I dare say there's a more idiomatic way of doing this that probably looks to me like line noise. :)

`g_date_time_to_unix` returns a `gint64`.  Since 27c0728 ("utils:
replace time_to_string with fmt-based formatting"), `parse_date_time`
has returned a `time_t`, which has historically been a 32-bit type.
There is, therefore, the possibility of integer over- and underflow.
Define `TIME_MIN` and `TIME_MAX` based on the size of `time_t` and clamp
the return value of `parse_date_time` between these.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
t = TIME_MAX;
if (t < TIME_MIN)
t = TIME_MIN;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::clamp

@djcb
Copy link
Owner

djcb commented Apr 13, 2024

Hmm, appreciate the effort, but I don't really want this... mu assumes time_t is 64-bit; if it isn't, your patch won't help getting the correct value for post-2038 dates.

So perhaps better to use 64-bit types everywhere rather than time_t, however, time_t makes the code easier to read.

So perhaps a bulld-time warning? Or even a run-time warning when starting mu?

@a3a3el
Copy link
Contributor Author

a3a3el commented Apr 13, 2024

I did look at going back to a 64-bit type, but libfmt expects time_t, so the problem would just have cropped up elsewhere. If you're happy assuming a 64-bit time_t, I'll close the PR. I shall probably carry some version of the patch in Debian, because there are a couple of obscure architectures which lack support for a 64-bit time_t.

@a3a3el a3a3el closed this Apr 13, 2024
@djcb
Copy link
Owner

djcb commented Apr 14, 2024

Okay... preferably we don't need a patch in Debian.. I've pushed a small change, more-or-less converting your change to in to mu-style c++ :-) .

Of course, it still won't quite work on a 32-bit platform once 2038 hits.

@a3a3el
Copy link
Contributor Author

a3a3el commented Apr 20, 2024 via email

@djcb
Copy link
Owner

djcb commented Apr 22, 2024

Ah, your change wasn't too different, just some minor C++ busywork :)

And yes, by 2038 hopefully all archs have switch to a 64-bit time_t; mu is probably the least of people's worries if they aren't...

@a3a3el a3a3el deleted the bugfix/time_t-overflow-fixes branch May 1, 2024 19:35
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.

None yet

2 participants