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 segmentation fault when datetime module is invalid #3818

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

davidhewitt
Copy link
Member

Reported downstream in pydantic/pydantic-core#1171

We aren't checking if PyDatetime_IMPORT() succeeds, which can lead to dereference of a null pointer and a crash if the import fails.

This PR fixes that crash by adding error checking. Where possible I return the error, otherwise I just panic.

@davidhewitt davidhewitt linked an issue Feb 10, 2024 that may be closed by this pull request
Copy link

codspeed-hq bot commented Feb 10, 2024

CodSpeed Performance Report

Merging #3818 will degrade performances by 16.95%

Comparing davidhewitt:datetime-segv (5b11041) with main (fa53d81)

Summary

⚡ 2 improvements
❌ 3 regressions
✅ 74 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main davidhewitt:datetime-segv Change
mapping_from_dict 300 ns 355.6 ns -15.63%
list_via_downcast 157.2 ns 185 ns -15.02%
sequence_from_list 272.2 ns 327.8 ns -16.95%
extract_float_extract_success 472.8 ns 417.2 ns +13.32%
extract_float_downcast_success 481.1 ns 425.6 ns +13.05%

src/types/datetime.rs Outdated Show resolved Hide resolved
src/types/datetime.rs Outdated Show resolved Hide resolved
Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Two nits. Otherwise LGTM.

I guess an invalid module on the path is weird enough that this does not need an immediate backport and 0.20.x maintenance release? This module did change significantly since 0.20.x IIRC.

@davidhewitt
Copy link
Member Author

Thanks for the review 👍

I guess an invalid module on the path is weird enough that this does not need an immediate backport and 0.20.x maintenance release? This module did change significantly since 0.20.x IIRC.

Yes, I think it is a weird edge case. I think it's also very likely that this will always just crash immediately without doing any harm, so it's just ugly UX rather than dangerous.

That said, I'm about to prepare a patch release for #3619, so if it's trivial to cherry-pick this I will.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 11, 2024
@davidhewitt davidhewitt added this pull request to the merge queue Feb 11, 2024
Merged via the queue into PyO3:main with commit 55488d3 Feb 11, 2024
35 of 38 checks passed
@davidhewitt davidhewitt deleted the datetime-segv branch February 11, 2024 09:44
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.

Segmentation fault - datetime python module
2 participants