BUG: Segfault in to_json with tzaware datetime #45558
Merged
Conversation
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
lithomas1
added
Bug
IO JSON
mroeschke
approved these changes
Jan 23, 2022
WillAyd
requested changes
Jan 23, 2022
| @@ -378,7 +378,7 @@ int convert_pydatetime_to_datetimestruct(PyObject *dtobj, | |||
| if (tmp == NULL) { | |||
| return -1; | |||
| } | |||
| seconds_offset = PyInt_AsLong(tmp); | |||
| seconds_offset = PyInt_AsLong(PyNumber_Long(tmp)); | |||
There was a problem hiding this comment.
PyNumber_Long returns a new reference, so the way this is currently written it will leak memory. You ideally should break this into a few lines and be sure to call Py_DECREF on the object returned from PyNumber_Long
While touching this might as well also switch from PyInt_AsLong to PyLong_AsLong
There was a problem hiding this comment.
While touching this might as well also switch from PyInt_AsLong to PyLong_AsLong
Will try to do this as a follow up as I'm planning to have this PR backported.
WillAyd
reviewed
Jan 23, 2022
| @@ -378,11 +379,14 @@ int convert_pydatetime_to_datetimestruct(PyObject *dtobj, | |||
| if (tmp == NULL) { | |||
| return -1; | |||
| } | |||
| seconds_offset = PyInt_AsLong(tmp); | |||
| tmp_int = PyNumber_Long(tmp); | |||
There was a problem hiding this comment.
You will also want to do a if (tmp_int == NULL) check and return an error / DECREF tmp if that happens. Otherwise lgtm
WillAyd
approved these changes
Jan 24, 2022
|
Thanks @lithomas1 awesome work |
meeseeksmachine
pushed a commit
to meeseeksmachine/pandas
that referenced
this pull request
Jan 24, 2022
jreback
pushed a commit
that referenced
this pull request
Jan 24, 2022
…45584) Co-authored-by: Thomas Li <[email protected]>
yehoshuadimarsky
pushed a commit
to yehoshuadimarsky/pandas
that referenced
this pull request
Jul 13, 2022
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.
Basically, timedelta.total_seconds returns a float. PyInt_AsLong(actually PyLong_AsLong) wants an int, but would cast using int for Pythons < 3.10. This stopped happening in python 3.10. (https://docs.python.org/3/c-api/long.html#c.PyLong_AsLong).
IMO, it probably is fine to cast the result from float to int, as I don't think that any timezone has an offset from UTC that contains fractions of a second. But, I could be wrong given that I don't use timezones and are not too familiar with them.