GeezerVison

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

GeezerVison is showing github.com — gh-106844: Fix null-bytes handling in LCMapStringEx in _winapi by serhiy-storchaka · Pull Request #107832 · python/cpython

Conversation

serhiy-storchaka commented Aug 10, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Based on #106857 with some fixes.

Issue: _winapi.LCMapStringEx fails when encountering a string containing null characters #106844

Sorry, something went wrong.

Uh oh!

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

All reactions

sobolevn and others added 5 commits

July 18, 2023 13:02


          pythongh-106844: Fix null-bytes handling in LCMapStringEx in _winapi

dff8b2f


          Fix CI

46be70b


          Keep simpler code for the locale parameter.

13ce79c


          Handle large strings containing the null character.

88d4193


          Merge branch 'main' into issue-106844

73551b9

serhiy-storchaka added needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Aug 10, 2023

serhiy-storchaka requested a review from zooba

August 10, 2023 08:41

bedevere-bot mentioned this pull request

Aug 10, 2023

_winapi.LCMapStringEx fails when encountering a string containing null characters #106844

Closed

bedevere-bot added the awaiting core review label

Aug 10, 2023

serhiy-storchaka mentioned this pull request

Aug 10, 2023

gh-106844: Fix null-bytes handling in LCMapStringEx in _winapi #106857

Closed

zooba approved these changes

Aug 10, 2023

View reviewed changes

zooba 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. Just be aware it won't backport cleanly to 3.11, and the LPCWSTR converter has a memory leak in that version and shouldn't be used.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Aug 10, 2023

serhiy-storchaka commented Aug 10, 2023

Copy link

Copy Markdown

Member Author

Before merging it I want to manually test it on Windows with large strings, and maybe add bigmem tests if it is feasible.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Fix other issues.

26161c3

serhiy-storchaka commented

Aug 11, 2023

View reviewed changes

Comment thread

Modules/_winapi.c

		NULL, NULL, 0);
		if (dest_size == 0) {
		return PyErr_SetFromWindowsErr(0);
		if (dest_size <= 0) {

serhiy-storchaka Aug 11, 2023

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.

It can return negative value. At least if src_size is negative. But the second call returns 0 anyway.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_winapi.c

		if (dest_size == 0) {
		return PyErr_SetFromWindowsErr(0);
		if (dest_size <= 0) {
		DWORD error = GetLastError();

serhiy-storchaka Aug 11, 2023

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.

Other code calls GetLastError() as early as possible, before PyMem_Free(). Perhaps there is a reason of it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_winapi.c

Comment on lines +1590 to +1591

		PyMem_Free(src_);
		PyObject *ret = PyUnicode_FromWideChar(dest, nmapped);

serhiy-storchaka Aug 11, 2023

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.

It reduces the maximal total memory consumption if free this memory before allocating new memory in PyUnicode_FromWideChar().

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka merged commit 04cc014 into python:main

Aug 11, 2023

miss-islington commented Aug 11, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the awaiting merge label

Aug 11, 2023

serhiy-storchaka deleted the issue-106844 branch

August 11, 2023 18:13

miss-islington commented Aug 11, 2023

Copy link

Copy Markdown

Contributor

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 04cc01453db2f0af72a06440831637f8bf512daf 3.12

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned serhiy-storchaka

Aug 11, 2023

miss-islington commented Aug 11, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Aug 11, 2023


          [3.12] pythongh-106844: Fix issues in _winapi.LCMapStringEx (pythonGH…

21a0b8d

…-107832)

* Strings with length from 2**31-1 to 2**32-2 always caused MemoryError,
   it doesn't matter how much memory is available.
* Strings with length exactly 2**32-1 caused OSError.
* Strings longer than 2**32-1 characters were truncated due to integer overflow bug.
* Strings containing the null character were truncated at the first null character.

Now strings longer than 2**31-1 characters caused OverflowError and the null character is allowed..
(cherry picked from commit 04cc014)

Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot commented Aug 11, 2023

Copy link

Copy Markdown

GH-107874 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.12 only security fixes label

Aug 11, 2023

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Aug 11, 2023


          [3.11] pythongh-106844: Fix issues in _winapi.LCMapStringEx (pythonGH…

a1a91e7

…-107832)

* Strings with length from 2**31-1 to 2**32-2 always caused MemoryError,
   it doesn't matter how much memory is available.
* Strings with length exactly 2**32-1 caused OSError.
* Strings longer than 2**32-1 characters were truncated due to integer overflow bug.
* Strings containing the null character were truncated at the first null character.

Now strings longer than 2**31-1 characters caused OverflowError and the null character is allowed..
(cherry picked from commit 04cc014)

Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot commented Aug 11, 2023

Copy link

Copy Markdown

GH-107875 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.11 only security fixes label

Aug 11, 2023

sobolevn commented Aug 11, 2023

Copy link

Copy Markdown

Member

Thanks for fixing this! 👍

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka added a commit that referenced this pull request

Aug 12, 2023


          [3.11] gh-106844: Fix issues in _winapi.LCMapStringEx (GH-107832) (GH…

ec254c5

…-107875)

* Strings with length from 2**31-1 to 2**32-2 always caused MemoryError,
   it doesn't matter how much memory is available.
* Strings with length exactly 2**32-1 caused OSError.
* Strings longer than 2**32-1 characters were truncated due to integer overflow bug.

Now strings longer than 2**31-1 characters caused OverflowError.
(cherry picked from commit 04cc014)

finnagin mentioned this pull request

Aug 14, 2023

gh-106242: Fix path truncation in normpath #106816

Merged

zooba pushed a commit to zooba/cpython that referenced this pull request

Aug 15, 2023


          pythongh-106844: Fix issues in _winapi.LCMapStringEx (pythonGH-107832)

5d83c72

* Strings with length from 2**31-1 to 2**32-2 always caused MemoryError,
   it doesn't matter how much memory is available.
* Strings with length exactly 2**32-1 caused OSError.
* Strings longer than 2**32-1 characters were truncated due to integer overflow bug.
* Strings containing the null character were truncated at the first null character.

Now strings longer than 2**31-1 characters caused OverflowError and the null character is allowed.

Yhg1s pushed a commit that referenced this pull request

Aug 16, 2023


          [3.12] gh-106844: Fix issues in _winapi.LCMapStringEx (GH-107832) (#1…

b5176a8

…07874)

* Strings with length from 2**31-1 to 2**32-2 always caused MemoryError,
   it doesn't matter how much memory is available.
* Strings with length exactly 2**32-1 caused OSError.
* Strings longer than 2**32-1 characters were truncated due to integer overflow bug.
* Strings containing the null character were truncated at the first null character.

Now strings longer than 2**31-1 characters caused OverflowError and the null character is allowed..
(cherry picked from commit 04cc014)

serhiy-storchaka removed their assignment

Sep 8, 2023

Reviewers

zooba

zooba approved these changes

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.

Uh oh!

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

5 participants

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.

GeezerVison

Conversation

serhiy-storchaka commented Aug 10, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 10, 2023

Uh oh!

serhiy-storchaka Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 11, 2023

Uh oh!

miss-islington commented Aug 11, 2023

Uh oh!

miss-islington commented Aug 11, 2023

Uh oh!

bedevere-bot commented Aug 11, 2023

Uh oh!

bedevere-bot commented Aug 11, 2023

Uh oh!

sobolevn commented Aug 11, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Aug 10, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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