GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-52551: Fix encoding issues in strftime() by serhiy-storchaka · Pull Request #125193 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Oct 9, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Closes #52551, #78662 and #124531.

  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
pythongh-52551: Fix encoding issues in strftime()
c9f1156 
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 serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Oct 9, 2024
@serhiy-storchaka
serhiy-storchaka requested review from abalkin and pganssle as code owners October 9, 2024 14:25
@bedevere-app bedevere-app Bot mentioned this pull request Oct 9, 2024
time.strftime() and Unicode characters on Windows #52551
Closed
@bedevere-app bedevere-app Bot 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 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 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-app bedevere-app Bot mentioned this pull request Oct 9, 2024
datetime.strftime strings can be terminated by "\x00" literals #124531
Closed
@serhiy-storchaka serhiy-storchaka changed the title gh-124531: Fix encoding issues in strftime() gh-78662: Fix encoding issues in strftime() Oct 9, 2024
@bedevere-app bedevere-app Bot 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 serhiy-storchaka changed the title gh-78662: Fix encoding issues in strftime() gh-52551: Fix encoding issues in strftime() Oct 9, 2024
@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 9, 2024
gh-124531: Fix strftime() with embedded null characters #125003
Closed
@serhiy-storchaka
serhiy-storchaka requested a review from vstinner October 10, 2024 16:24
@serhiy-storchaka

serhiy-storchaka commented Oct 10, 2024

Copy link
Copy Markdown
Member Author

@vstinner, could you please review this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Oct 16, 2024
View reviewed changes
Comment thread Modules/_datetimemodule.c Outdated
newformat, timetuple, NULL);
Py_DECREF(newformat);
}
if (0) {

@vstinner vstinner Oct 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Oct 17, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_datetimemodule.c Outdated
}
newformat = _PyUnicodeWriter_Finish(&writer);
}
if (newformat != NULL) {

@vstinner vstinner Oct 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would prefer to exit early: if (newformat == NULL) goto ....

By the way, only the _PyUnicodeWriter_Finish() code path can fail, no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Oct 17, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/timemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/timemodule.c Outdated
* will be ahead of time...
*/
while (1) {
outbuf = (time_char *)PyMem_Realloc(outbuf, bufsize*sizeof(time_char));

@vstinner vstinner Oct 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You may also add an explicit integer overflow check here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/timemodule.c Outdated
Py_DECREF(unicode);
}

j = i;

@vstinner vstinner Oct 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/timemodule.c
}
format[fmtlen++] = (char)c;
}
if (fmtlen) {

@vstinner vstinner Oct 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Oct 17, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 3 commits October 17, 2024 14:43
@serhiy-storchaka
Merge branch 'main' into strftime
690e64a
@serhiy-storchaka
Address some review comments.
c75eb0c
@serhiy-storchaka
Address other review comment.
ca215dd
@serhiy-storchaka

serhiy-storchaka commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Oct 17, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/timemodule.c Outdated
Comment on lines +825 to +826
*outbuf = (time_char *)PyMem_Realloc(*outbuf,
*bufsize*sizeof(time_char));

@vstinner vstinner Oct 17, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/timemodule.c Outdated

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

@vstinner vstinner Oct 17, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 17, 2024
@serhiy-storchaka
Apply style suggestions.
d4fcbf3
@serhiy-storchaka

serhiy-storchaka commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

Thank you for your review @vstinner.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) October 17, 2024 15:19
@serhiy-storchaka
serhiy-storchaka merged commit ad3eac1 into python:main Oct 17, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 17, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka Oct 17, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka deleted the strftime branch October 17, 2024 16:18
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 17, 2024
@serhiy-storchaka
[3.13] pythongh-52551: Fix encoding issues in strftime() (pythonGH-12…
13ef787 
…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 <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 17, 2024
@serhiy-storchaka
pythongh-52551: Fix encoding issues in strftime() (pythonGH-125193)
4019173 
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

bedevere-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Oct 17, 2024
@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.12 only security fixes label Oct 17, 2024
@serhiy-storchaka serhiy-storchaka removed their assignment Oct 17, 2024
serhiy-storchaka added a commit that referenced this pull request Oct 17, 2024
@serhiy-storchaka
[3.13] gh-52551: Fix encoding issues in strftime() (GH-125193) (GH-12…
08ccbb9 
…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)
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 17, 2024
@serhiy-storchaka @miss-islington
[3.13] pythongh-52551: Fix encoding issues in strftime() (pythonGH-12…
a34caba 
…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 <[email protected]>
(cherry picked from commit ad3eac1)
serhiy-storchaka added a commit that referenced this pull request Oct 17, 2024
@miss-islington @serhiy-storchaka
[3.12] gh-52551: Fix encoding issues in strftime() (GH-125193) (GH-12…
232b303 
…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 <[email protected]>
@vstinner

vstinner commented Oct 18, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@serhiy-storchaka @ebonnal
pythongh-52551: Fix encoding issues in strftime() (pythonGH-125193)
3a7c158 
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.
@rlamy rlamy mentioned this pull request Oct 17, 2026
time.strftime: only pass ASCII runs of the format to wcsftime() pypy/pypy#5665
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@abalkin abalkin Awaiting requested review from abalkin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

datetime.strftime strings can be terminated by "\x00" literals time.strftime() and Unicode characters on Windows

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@serhiy-storchaka @vstinner
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.