lib/datetime: Fix reliability issues and replace non-reentrant time functions#7597
Draft
NeelGhoshal wants to merge 4 commits into
Draft
lib/datetime: Fix reliability issues and replace non-reentrant time functions#7597NeelGhoshal wants to merge 4 commits into
NeelGhoshal wants to merge 4 commits into
Conversation
…Time" contains padding.
…ltime" by a call to "localtime_r".
…me" by a call to "gmtime_r".
…altime" by a call to "localtime_r".
Comment on lines
+24
to
+32
| return src->year == dst->year && | ||
| src->month == dst->month && | ||
| src->day == dst->day && | ||
| src->hour == dst->hour && | ||
| src->minute == dst->minute && | ||
| src->second == dst->second && | ||
| src->usec == dst->usec && | ||
| src->timezone == dst->timezone; | ||
|
|
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| return src->year == dst->year && | |
| src->month == dst->month && | |
| src->day == dst->day && | |
| src->hour == dst->hour && | |
| src->minute == dst->minute && | |
| src->second == dst->second && | |
| src->usec == dst->usec && | |
| src->timezone == dst->timezone; | |
| return src->year == dst->year && src->month == dst->month && | |
| src->day == dst->day && src->hour == dst->hour && | |
| src->minute == dst->minute && src->second == dst->second && | |
| src->usec == dst->usec && src->timezone == dst->timezone; |
Member
|
Since you’re using an AI assistance, could you try harder to add some "unit" tests for this, and after that also your other recent PRs? Ideally in pytest (since we have the architecture for it), if the functions are public, so available in the ctypes bindings. You can’t really assume that the pre-existing state is tested well enough |
Contributor
|
You obviously didn’t test locally, read CONTRIBUTING.md, and do not waste others time. (Pun intended). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix a reliability bug and replace two non-reentrant time functions in
lib/datetime.same.cDateTimestructs member-by-member instead of withmemcmp; the struct contains padding bytes whose values are unspecified, makingmemcmpincorrect for equality checkslocal.clocaltime()withlocaltime_r()(×2)local.cgmtime()withgmtime_r()Motivation and context
The
memcmpequality check on a struct with padding is undefined behaviour —padding bytes are not guaranteed to be zeroed, so two logically equal
DateTimevalues can compare as unequal. Member-by-member comparisonavoids this.
localtime()andgmtime()return pointers to a shared static buffer,making them unsafe in multithreaded code. The
_rvariants write to acaller-supplied buffer.
How has this been tested?
lib/datetimehas no dedicated C unit test suite. The functions areexercised indirectly through the temporal module and related tools.
These changes are behaviour-preserving for single-threaded use and
correctness-fixing for the struct comparison.
Types of changes
Checklist