classification
Title: No error checking after using of the wcsxfrm()
Type: behavior Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: christian.heimes, loewis, serhiy.storchaka, vstinner
Priority: low Keywords: patch

Created on 2012-09-17 10:14 by serhiy.storchaka, last changed 2017-03-24 22:47 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
wcsxfrm_error_check.patch serhiy.storchaka, 2012-09-17 11:40 review
Pull Requests
URL Status Linked Edit
PR 508 merged serhiy.storchaka, 2017-03-06 10:23
Messages (13)
msg170592 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-17 10:14
The wcsxfrm() function may fail but there is no error check code.

I don't know what exception type should be used here: OSError with an errno EINVAL or some specialized type. I can't prepare tests, because I don't know under what conditions it would be possible error, and whether we can get it on any system at all without broking some internal unicode object invariants.
msg170593 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-17 10:22
Have you tried code points beyond the BMP?

The C function doesn't have a return value that signals an error. An explicit check of errno is required. http://linux.die.net/man/3/wcsxfrm
msg170595 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-17 11:40
Here is a sample patch (to specify the location of the issue). I don't sure OSError is well here.

As far as I understand, this function is Windows-specific, so I can't check how it works with code points beyond the BMP or with surrogates.
msg170596 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-17 11:48
The patch looks good. `s` and `buf` are cleaned up after the exit label.
msg170601 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-09-17 12:35
> I don't know what exception type should be used here: 
> OSError with an errno EINVAL or some specialized type. 

No, I think the appropriate error is ValueError, at least if errno is EINVAL.

> because I don't know under what conditions it would be possible error

AFAICT, glibc never sets errno. 

msvcrt gives EILSEQ or ERANGE, but never EINVAL. EILSEQ is returned if LCMapString failed, and ERANE if the output buffer is too small. In either case, the result is INT_MAX.
msg170814 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-20 14:53
> No, I think the appropriate error is ValueError, at least if errno is EINVAL.

With what message?

> msvcrt gives EILSEQ or ERANGE, but never EINVAL. EILSEQ is returned if LCMapString failed, and ERANE if the output buffer is too small.

I don't see where ERANE can be returned. If the output buffer is too small then the required buffer size (not including terminating null-char) should be returned.

I see the same issue with wcscoll() in locale.strcoll().

An alternative solution is in case of an error to return the original string (in locale.strxfrm) and to compare strings without regard locale (in locale.strcoll).
msg171197 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-09-24 21:41
Dummy question: can you provide an example of strings that make wcsxfrm() failing?
msg171200 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-24 22:10
> Dummy question: can you provide an example of strings that make wcsxfrm() failing?

No, I can not (on Linux).
msg171201 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-24 22:18
What error message is most appropriate here?

Can we return the original string instead of exception?
msg220635 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-15 13:25
@Serhiy will you follow up on this?
msg220667 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-06-15 19:44
I have no Windows and can't provide relevant test case.
msg220672 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-15 20:48
@Serhiy you don't need Windows, msg170593 refers to a Linux man page.  Reading your msg170595 I'd guess that you've got confused with a similar function that is Windows specific.
msg290284 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 22:47
New changeset be487a65f18e1be5fde03e2977fff4be53cc2fbf by Serhiy Storchaka in branch 'master':
bpo-15954: Check return code of wcsxfrm(). (#508)
https://github.com/python/cpython/commit/be487a65f18e1be5fde03e2977fff4be53cc2fbf
History
Date User Action Args
2017-03-24 22:47:05serhiy.storchakasetmessages: + msg290284
2017-03-06 19:23:12serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.7, - Python 3.4, Python 3.5
2017-03-06 18:17:04BreamoreBoysetnosy: - BreamoreBoy
2017-03-06 10:23:20serhiy.storchakasetpull_requests: + pull_request417
2014-06-15 20:48:24BreamoreBoysetmessages: + msg220672
2014-06-15 19:44:32serhiy.storchakasetmessages: + msg220667
2014-06-15 13:25:09BreamoreBoysetnosy: + BreamoreBoy

messages: + msg220635
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2012-09-24 22:18:54serhiy.storchakasetmessages: + msg171201
2012-09-24 22:10:30serhiy.storchakasetmessages: + msg171200
2012-09-24 21:41:11vstinnersetmessages: + msg171197
2012-09-20 14:53:08serhiy.storchakasetmessages: + msg170814
2012-09-18 21:50:52vstinnersetnosy: + vstinner
2012-09-17 12:35:18loewissetmessages: + msg170601
2012-09-17 11:48:49christian.heimessetmessages: + msg170596
stage: patch review
2012-09-17 11:40:33serhiy.storchakasetfiles: + wcsxfrm_error_check.patch
keywords: + patch
messages: + msg170595
2012-09-17 10:22:32christian.heimessetnosy: + christian.heimes
messages: + msg170593
2012-09-17 10:14:33serhiy.storchakacreate