Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add check if str(handle) correctly converted the object, and throw py::error_already_set if not #2392

Merged
merged 2 commits into from
Aug 17, 2020
Merged

Add check if str(handle) correctly converted the object, and throw py::error_already_set if not #2392

merged 2 commits into from
Aug 17, 2020

Conversation

YannickJadoul
Copy link
Collaborator

Only problematic on Python 2, I think. Discovered here: #2380 (comment)

This was referenced Aug 14, 2020
Merged
Closed
@YannickJadoul
Copy link
Collaborator Author

Also note that by checking always checking the result after calling raw_ptr, we can more the if (!str_value) throw error_already_set(); inside the Python 2-only preprocessor block, and get rid of one full pointer comparison (!) on Python 3, when calling the constructor from py::object! ;-)

@YannickJadoul
Copy link
Collaborator Author

Of course. I tried this locally based on #2380, but detail::PyUnicode_Check_Permissive first still needs to die, such that py::str cannot contain a bytes-object anymore!

@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from 235168a to d58307d Compare August 14, 2020 00:39
@YannickJadoul
Copy link
Collaborator Author

Rebased onto #2380, but waiting for #2380 to get merged before this can get rebased and merged.

YannickJadoul
YannickJadoul commented Aug 14, 2020
View reviewed changes
tests/test_pytypes.py Outdated Show resolved Hide resolved
@rwgk rwgk force-pushed the str-bytes-cleanup branch 2 times, most recently from f38fafc to 6cd4552 Compare August 14, 2020 22:03
@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch 3 times, most recently from 7eee06e to 366f713 Compare August 15, 2020 21:51
@YannickJadoul YannickJadoul self-assigned this Aug 15, 2020
bstaletic
bstaletic reviewed Aug 16, 2020
View reviewed changes
Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Looks good, apart from that sys.version.major thing.

@rwgk rwgk force-pushed the str-bytes-cleanup branch from 1d553e4 to 0b49467 Compare August 16, 2020 17:24
@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from 366f713 to 6452317 Compare August 16, 2020 17:41
@henryiii
Copy link
Collaborator

henryiii commented Aug 16, 2020 •
edited
Loading

@YannickJadoul if you used pre-commit install this couldn't happen. 😜

@henryiii
Copy link
Collaborator

henryiii commented Aug 16, 2020 •
edited
Loading

This might benefit from being rebased after #2397

@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from 6452317 to b25eace Compare August 16, 2020 17:47
henryiii
henryiii reviewed Aug 16, 2020
View reviewed changes
tests/test_pytypes.py Outdated Show resolved Hide resolved
henryiii
henryiii reviewed Aug 16, 2020
View reviewed changes
include/pybind11/cast.h Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Collaborator Author

@YannickJadoul if you used pre-commit install this couldn't happen. stuck_out_tongue_winking_eye

OK, you win this one ;-)

@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from b25eace to f7d376c Compare August 16, 2020 21:13
@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from f7d376c to 8e53b83 Compare August 16, 2020 21:29
@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from 8e53b83 to 3dbafa6 Compare August 16, 2020 21:30
@YannickJadoul
Copy link
Collaborator Author

Fixed this. It's quite a minor PR, so a few more quick checks (@bstaletic & @henryiii, the things you mentioned should be fixed).
@rwgk, maybe you should give this a quick final check (since it involves py::str), and merge into str-bytes-cleanup at will if you're happy with it?

@wjakob
Copy link
Member

wjakob commented Aug 17, 2020

LGTM.

rwgk
rwgk approved these changes Aug 17, 2020
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Yannick!
I'll hold off merging only because I'm still unclear why the current str-bytes-cleanup branch fails to pass some tests in the Google environment (I didn't get a chance to work on it at all yesterday).
(For completeness: the failing tests are NOT pybind11 unit tests.)

@rwgk
Copy link
Collaborator

rwgk commented Aug 17, 2020

Looks great, thanks Yannick!
I'll hold off merging only because I'm still unclear why the current str-bytes-cleanup branch fails to pass some tests in the Google environment (I didn't get a chance to work on it at all yesterday).
(For completeness: the failing tests are NOT pybind11 unit tests.)

For future reference: the test failure was due to accidental str vs bytes mixup, masked by the double-nature of pybind11::str.

@rwgk rwgk merged commit 7ae6bb2 into pybind:str-bytes-cleanup Aug 17, 2020
@YannickJadoul YannickJadoul deleted the fix-python2-str-malformed_utf8 branch August 17, 2020 22:08
@henryiii henryiii added the bug label Aug 19, 2020
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 8, 2020
Adds check if str(handle) correctly converted the object, and throw…
d629d5e 
… py::error_already_set if not.

Similar to pybind#2392, but does not depend on pybind#2409.

Splitting out this PR from pybind#2409 to make that PR as simple as possible.

Net effects of this PR:
* Adds missing test coverage.
* Changes TypeError to UnicodeDecodeError for Python 2.

This PR has two commits. Please do not squash, to make the behavior change obvious in the commit history.
@rwgk rwgk mentioned this pull request Sep 8, 2020
Closed
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 10, 2020
Backing out changes originating from pybind#2392.
6f270a3
YannickJadoul added a commit to YannickJadoul/pybind11 that referenced this pull request Sep 10, 2020
@YannickJadoul
Apply @rwgk's fixes to cherry-picked commits from pybind#2392
76f3ebe
@YannickJadoul YannickJadoul mentioned this pull request Sep 10, 2020
Merged
YannickJadoul added a commit to YannickJadoul/pybind11 that referenced this pull request Sep 11, 2020
@YannickJadoul
Apply @rwgk's fixes to cherry-picked commits from pybind#2392
055f86f
YannickJadoul added a commit that referenced this pull request Sep 11, 2020
@YannickJadoul
Add check if str(handle) correctly converted the object, and throw …
fe9ee86 
…py::error_already_set if not (bis) (#2477)

* Add check if `str(handle)` correctly converted the object, and throw py::error_already_set if not

* Fix tests on Python 3

* Apply @rwgk's fixes to cherry-picked commits from #2392
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 16, 2020
Backing out changes originating from pybind#2392.
bd1d77b
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 16, 2020
Backing out changes originating from pybind#2392.
21376dd
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 27, 2020
Backing out changes originating from pybind#2392.
1bdb07d
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Oct 13, 2020
Backing out changes originating from pybind#2392.
1152249
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Oct 18, 2020
Backing out changes originating from pybind#2392.
924502a
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Oct 29, 2020
Backing out changes originating from pybind#2392.
df6e916
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henryiii henryiii henryiii left review comments

@bstaletic bstaletic bstaletic approved these changes

@rwgk rwgk rwgk approved these changes

Assignees

@YannickJadoul YannickJadoul

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@YannickJadoul @henryiii @wjakob @rwgk @bstaletic