Skip to content

Conversation

@bmerry
Copy link
Contributor

@bmerry bmerry commented Oct 10, 2021

Description

If an iterator returns a pair<T1, T2> rather than a reference to a pair or a pair of references, make_key_iterator and make_value_iterator would return a reference to a temporary, typically leading to a segfault. This is because the value category of member access to a prvalue is an xvalue, not a prvalue, so decltype produces an rvalue reference type. Fix the type calculation to handle this case.

I also removed some decltype parentheses that weren't needed, either because the expression isn't one of the special cases for decltype or because decltype was only used for SFINAE. Hopefully that makes the code a bit more readable.

Closes #3347

Suggested changelog entry:

Fix a regression in 2.8.0 that caused undefined behavior (typically segfaults) in ``make_key_iterator``/``make_value_iterator`` if dereferencing the iterator returned a temporary value instead of a reference.

If the iterator dereference operator returns a value rather than a
reference (and that pair also does not *contain* references),
make_key_iterator and make_value_iterator will return a reference to a
temporary, causing a segfault.
If an iterator returns a pair<T1, T2> rather than a reference to a pair
or a pair of references, make_key_iterator and make_value_iterator would
return a reference to a temporary, typically leading to a segfault. This
is because the value category of member access to a prvalue is an
xvalue, not a prvalue, so decltype produces an rvalue reference type.
Fix the type calculation to handle this case.

I also removed some decltype parentheses that weren't needed, either
because the expression isn't one of the special cases for decltype or
because decltype was only used for SFINAE. Hopefully that makes the code
a bit more readable.

Closes pybind#3347
@bmerry
Copy link
Contributor Author

bmerry commented Oct 10, 2021

cc @henryiii @rwgk @rainwoodman since you reviewed #3271.

@bmerry
Copy link
Contributor Author

bmerry commented Oct 10, 2021

Argh, why is there always a compiler bug?! Looks like I'm going to have to come up with a workaround for MSVC, but I probably won't have time for that today.

An alternative idea I had, but couldn't quite make work, is to have the access helper classes return a py::object, passing (*it).second or whatever directly into py::cast so that there is no need to figure out what the type is (it would also let the caster operate directly on a temporary returned from the iterator without needing to move/copy it). I wasn't sure how to get the casting to exactly match what the built-in return type conversion does, in terms of handling return value policies and special cases like the iterator already returning a py::object.

@Skylion007
Copy link
Collaborator

@bmerry It's not a MSVC issue, it's a CUDA compiler issue on Ubuntu 20. It seems like it's emitting an error for the same UB that this PR is suppose to be guarding against.

@bmerry
Copy link
Contributor Author

bmerry commented Oct 10, 2021

Sorry, MSVC was a typo - I had seen it was an CUDA issue. Apparently implementing decltype correctly is hard: I've now seen (different) implementation bugs in MSVC, ICC and nvcc.

@bmerry
Copy link
Contributor Author

bmerry commented Oct 10, 2021

Ok, I've found a workaround for the nvcc issue, and also filed bug 3399127 with Nvidia. There are still a few CI jobs running but I'm optimistic.

@rwgk
Copy link
Collaborator

rwgk commented Oct 11, 2021

I applied this PR on top of the smart_holder branch, along with updates from master, and manually ran all tests/ with asan, msan, ubsan, tsan (for completeness: this does not include the embedding tests). There were no new issues (only 2 known ubsan failures, 1 known tsan failure).

I also initiated google global testing. Results are expected by tomorrow morning (pacific time).

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.

Google global testing was successful.
(I didn't review in detail.)

@rwgk rwgk merged commit 8a7c266 into pybind:master Oct 11, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 11, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: new make_key_iterator/make_value_iterator in 2.8.0 may return reference to temporary

4 participants