gh-52551: Fix encoding issues in strftime() by serhiy-storchaka · Pull Request #125193 · python/cpython

serhiy-storchaka · 2024-10-09T14:25:06Z

Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time.

Characters not encodable in the current locale are now acceptable in the format string.
Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated.
Embedded null character no longer terminates the format string.

Issue: time.strftime() and Unicode characters on Windows #52551

Issue: Different behavior of C and Python impls of datetime.strftime with non-UTF-8-encodable strings #78662

Issue: datetime.strftime strings can be terminated by "\x00" literals #124531

Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also pythongh-78662 and pythongh-124531.

serhiy-storchaka · 2024-10-10T16:25:42Z

@vstinner, could you please review this PR?

vstinner · 2024-10-16T12:12:59Z

+ newformat, timetuple, NULL);
+ Py_DECREF(newformat);
+ }
+ if (0) {


This if (0) is weird. Why not adding a label after _PyUnicodeWriter_Dealloc() and using goto?

This would require changing more code. Renaming label Done to Error (there are many goto Done in not changed code) and adding new Done label. The if (0) idiom is weird, but used in many places.

vstinner · 2024-10-16T12:14:23Z

 }
+ newformat = _PyUnicodeWriter_Finish(&writer);
 }
+ if (newformat != NULL) {


I would prefer to exit early: if (newformat == NULL) goto ....
By the way, only the _PyUnicodeWriter_Finish() code path can fail, no?

This would require adding more labels. The flow would look more complicated.

vstinner · 2024-10-16T12:39:59Z

+ * will be ahead of time...
+ */
+ while (1) {
+ outbuf = (time_char *)PyMem_Realloc(outbuf, bufsize*sizeof(time_char));


You may also add an explicit integer overflow check here.

vstinner · 2024-10-16T12:50:17Z

+ Py_DECREF(unicode);
+ }
+
+ j = i;


You should declare the j variable here, and I suggest to rename it to start.

vstinner · 2024-10-16T12:53:22Z

+ }
+ format[fmtlen++] = (char)c;
+ }
+ if (fmtlen) {


This function is quite big. You might move the code handling format in a sub-function (the whole if block).

This may be inconvenient, because this code changes several outer variables.: outbuf and bufsize.

serhiy-storchaka · 2024-10-17T14:01:25Z

I addressed all comments. Perhaps it is now more difficult to review.

vstinner

LGTM. I just left minor coding style suggestions that you can ignore.

Thanks for the exhaustive added tests, it's very helpful.

vstinner · 2024-10-17T14:10:49Z

+ *outbuf = (time_char *)PyMem_Realloc(*outbuf,
+ *bufsize*sizeof(time_char));


Suggested change
*outbuf= (time_char*)PyMem_Realloc(*outbuf,
*bufsize*sizeof(time_char));
*outbuf= (time_char*)PyMem_Realloc(*outbuf,
*bufsize*sizeof(time_char));

vstinner · 2024-10-17T14:12:38Z


 static PyObject *
-time_strftime(PyObject *module, PyObject *args)
+strftime1(time_char **outbuf, size_t *bufsize,


nitpick: you might rename the function to time_strftime1() to avoid any conflict with a potential strftime1() name from the C library.

serhiy-storchaka · 2024-10-17T15:18:31Z

Thank you for your review @vstinner.

miss-islington-app · 2024-10-17T15:47:02Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington-app · 2024-10-17T15:47:05Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ad3eac1963a5f195ef9b2c1dbb5e44fa3cce4c72 3.13

miss-islington-app · 2024-10-17T15:47:21Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ad3eac1963a5f195ef9b2c1dbb5e44fa3cce4c72 3.12

…5193) Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also pythongh-78662 and pythongh-124531. (cherry picked from commit ad3eac1) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also pythongh-78662 and pythongh-124531. (cherry picked from commit ad3eac1)

bedevere-app · 2024-10-17T17:21:10Z

GH-125657 is a backport of this pull request to the 3.13 branch.

…5657) Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also gh-78662 and gh-124531. (cherry picked from commit ad3eac1)

…5193) (pythonGH-125657) Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also pythongh-78662 and pythongh-124531. (cherry picked from commit 08ccbb9) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit ad3eac1)

…5657) (GH-125661) Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also gh-78662 and gh-124531. (cherry picked from commit 08ccbb9) (cherry picked from commit ad3eac1) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

vstinner · 2024-10-18T09:03:01Z

Congrats! That's a nice change. Using Unicode instead of bytes for the internal buffer is a good move.

Fix time.strftime(), the strftime() method and formatting of the datetime classes datetime, date and time. * Characters not encodable in the current locale are now acceptable in the format string. * Surrogate pairs and sequence of surrogatescape-encoded bytes are no longer recombinated. * Embedded null character no longer terminates the format string. This fixes also pythongh-78662 and pythongh-124531.

serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 9, 2024

serhiy-storchaka requested review from abalkin and pganssle as code owners October 9, 2024 14:25

bedevere-appBot mentioned this pull request Oct 9, 2024
time.strftime() and Unicode characters on Windows #52551
Closed

bedevere-appBot added the awaiting core review label Oct 9, 2024

This was linked to issues Oct 9, 2024
time.strftime() and Unicode characters on Windows #52551
Closed
datetime.strftime strings can be terminated by "\x00" literals #124531
Closed

serhiy-storchaka changed the title ~~gh-52551: Fix encoding issues in strftime()~~gh-52551, gh-78662, gh-124531: Fix encoding issues in strftime()Oct 9, 2024

serhiy-storchaka changed the title ~~gh-52551, gh-78662, gh-124531: Fix encoding issues in strftime()~~gh-124531: Fix encoding issues in strftime()Oct 9, 2024

bedevere-appBot mentioned this pull request Oct 9, 2024
datetime.strftime strings can be terminated by "\x00" literals #124531
Closed

serhiy-storchaka changed the title ~~gh-124531: Fix encoding issues in strftime()~~gh-78662: Fix encoding issues in strftime()Oct 9, 2024

bedevere-appBot mentioned this pull request Oct 9, 2024
Different behavior of C and Python impls of datetime.strftime with non-UTF-8-encodable strings #78662
Closed

serhiy-storchaka changed the title ~~gh-78662: Fix encoding issues in strftime()~~gh-52551: Fix encoding issues in strftime()Oct 9, 2024

serhiy-storchaka mentioned this pull request Oct 9, 2024
gh-124531: Fix strftime() with embedded null characters #125003
Closed

serhiy-storchaka requested a review from vstinner October 10, 2024 16:24

vstinner reviewed Oct 16, 2024
View reviewed changes

serhiy-storchaka added 3 commits October 17, 2024 14:43

Merge branch 'main' into strftime
690e64a

Address some review comments.
c75eb0c

Address other review comment.
ca215dd

vstinner approved these changes Oct 17, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting core review labels Oct 17, 2024

Apply style suggestions.
d4fcbf3

serhiy-storchaka enabled auto-merge (squash) October 17, 2024 15:19

serhiy-storchaka merged commit ad3eac1 into python:mainOct 17, 2024

bedevere-appBot removed the awaiting merge label Oct 17, 2024

miss-islington-appBot assigned serhiy-storchaka Oct 17, 2024

serhiy-storchaka deleted the strftime branch October 17, 2024 16:18

bedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Oct 17, 2024

serhiy-storchaka removed the needs backport to 3.12 only security fixes label Oct 17, 2024

serhiy-storchaka removed their assignment Oct 17, 2024

		outbuf = (time_char )PyMem_Realloc(*outbuf,
		bufsizesizeof(time_char));

Uh oh!

Uh oh!

Conversation

serhiy-storchaka commented Oct 9, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Oct 17, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Oct 17, 2024

Uh oh!

miss-islington-appBot commented Oct 17, 2024

Uh oh!

miss-islington-appBot commented Oct 17, 2024

Uh oh!

miss-islington-appBot commented Oct 17, 2024

Uh oh!

bedevere-appBot commented Oct 17, 2024

Uh oh!

vstinner commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

serhiy-storchaka commented Oct 9, 2024•
edited
Loading