NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

sobolevn wants to merge6 commits intopython:mainfromsobolevn:issue-106844

Closed

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

sobolevn wants to merge6 commits intopython:mainfromsobolevn:issue-106844

Conversation

Copy link

Member

sobolevn commentedJul 18, 2023•
edited by bedevere-bot
Loading

I've changed the clinic definition + backported changes made by@zooba in927b5af#diff-42415407f8d0ef2d42e29d13d979f633e3543770e62c3871e1101ad532d336a8

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

pythongh-106844: Fix null-bytes handling inLCMapStringExin_winapi

dff8b2f

bedevere-bot added the awaiting review label

Jul 18, 2023

bedevere-bot mentioned this pull request

Jul 18, 2023

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

Closed

Fix CI

46be70b

serhiy-storchaka reviewed

Jul 18, 2023

View reviewed changes

Modules/_winapi.c Outdated

		_winapi.LCMapStringEx

		locale:LPCWSTR
		locale:unicode

Copy link

Member

Choose a reason for hiding this comment

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

Why change it forlocale?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I just backported the solution from927b5af#diff-42415407f8d0ef2d42e29d13d979f633e3543770e62c3871e1101ad532d336a8R1518

So, 3.11 and 3.12 would match.

Copy link

Member

Choose a reason for hiding this comment

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

The code would be simpler if keep the LPCWSTR converter here.

Copy link

Member

Choose a reason for hiding this comment

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

I agree. We don't need to treat this argument any differently - it's only thesrc argument that requires special handling.

Modules/_winapi.c Outdated

Comment on lines 1567 to 1570

		srcLen= (int)srcLenAsSsize;
		if (srcLen!=srcLenAsSsize) {
		srcLen=-1;
		}

Copy link

Member

Choose a reason for hiding this comment

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

How would it work for large (more than2**32 characters) string containing NUL?

Copy link

Member

Choose a reason for hiding this comment

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

It doesn't, but that's the caller's responsibility.

Copy link

Member

Choose a reason for hiding this comment

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

LCMapStringEx() will simply return the result for the truncated input. It is bad. How is the caller expected to detect this problem if it only occurs in special circumferences?

What doesLCMapStringEx() return for large input not containing NUL? Most likely it fails. We should either emulate this response (raise an OSError with the same Windows error code) or simply raise an OverflowError.

serhiy-storchaka requested changes

Jul 19, 2023

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please address my comments. If the code in 3.11 has the same flaws, it should be fixed too.

Modules/_winapi.c Outdated

		_winapi.LCMapStringEx

		locale:LPCWSTR
		locale:unicode

Copy link

Member

serhiy-storchakaJul 19, 2023

Choose a reason for hiding this comment

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

The code would be simpler if keep the LPCWSTR converter here.

bedevere-bot added awaiting changes and removed awaiting review labels

Jul 19, 2023

Copy link

bedevere-bot commentedJul 19, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka added needs backport to 3.11

only security fixes

needs backport to 3.12only security fixes labels

Jul 19, 2023

Copy link

Member

zooba commentedAug 9, 2023

I don't think the fix is as simple as bringing forward this fix - the underlying code should be identical in current versions, it was just earlier versions that needed a different fix because of existing bugs in the Unicode converters.

I'll take a fresh look and see what ought to happen here. Chances are, updating theLPCWSTR clinic definition makes the most sense.

Copy link

Member

zooba commentedAug 9, 2023

Looks like the changes to make argument clinic supportPy_UNICODE_converter actually work when you opt to allow embedded nulls are... complex. I can't figure it out, and pretty sure if I did it'd have wide-ranging effects that we don't want to backport.

Leaving thelocale argument asLPCWSTR should be fine - we only need to handle embedded nulls in thesrc argument.