gh-106844: Fix null-bytes handling in `LCMapStringEx` in `_winapi` by sobolevn · Pull Request #106857 · python/cpython

sobolevn · 2023-07-18T10:03:58Z

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

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

serhiy-storchaka · 2023-07-18T13:24:03Z

 _winapi.LCMapStringEx

- locale: LPCWSTR
+ locale: unicode


Why change it for locale?

I just backported the solution from 927b5af#diff-42415407f8d0ef2d42e29d13d979f633e3543770e62c3871e1101ad532d336a8R1518
So, 3.11 and 3.12 would match.

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

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

serhiy-storchaka · 2023-07-18T13:27:21Z

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


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

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

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 does LCMapStringEx() 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

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

serhiy-storchaka · 2023-07-19T04:59:20Z

 _winapi.LCMapStringEx

- locale: LPCWSTR
+ locale: unicode


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

bedevere-bot · 2023-07-19T05:01:46Z

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 phrase I 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.

zooba · 2023-08-09T19:48:38Z

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 the LPCWSTR clinic definition makes the most sense.

zooba · 2023-08-09T22:10:37Z

Looks like the changes to make argument clinic support Py_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 the locale argument as LPCWSTR should be fine - we only need to handle embedded nulls in the src argument.

serhiy-storchaka · 2023-08-10T08:46:18Z

Opened #107832 for my propositions.

serhiy-storchaka · 2023-08-11T18:26:08Z

#107832 fixed also other issues.

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

serhiy-storchaka requested changes Jul 19, 2023
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jul 19, 2023

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jul 19, 2023

zooba removed the needs backport to 3.11 only security fixes label Aug 9, 2023

serhiy-storchaka added 3 commits August 10, 2023 11:26

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 mentioned this pull request Aug 10, 2023
gh-106844: Fix null-bytes handling in LCMapStringEx in _winapi #107832
Merged

Fix other issues.
26161c3

serhiy-storchaka closed this Aug 11, 2023

Uh oh!

Uh oh!

Conversation

sobolevn commented Jul 18, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 19, 2023

Uh oh!

zooba commented Aug 9, 2023

Uh oh!

zooba commented Aug 9, 2023

Uh oh!

serhiy-storchaka commented Aug 10, 2023

Uh oh!

serhiy-storchaka commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sobolevn commented Jul 18, 2023•
edited by bedevere-bot
Loading