Skip to content

shared_ptr<bool> vptr_deleter_armed_flag_ptr (instead of unique_ptr) #2882

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
Merged
merged 2 commits into from
Mar 3, 2021
Merged

shared_ptr<bool> vptr_deleter_armed_flag_ptr (instead of unique_ptr) #2882

merged 2 commits into from
Mar 3, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 3, 2021

Fixes heap-use-after-free bug.

This fix comes at the cost of using shared_ptr<bool> instead of unique_ptr.

AddressSanitizer errors for the two newly added tests, before changes to smart_holder_poc.h:

SUMMARY: AddressSanitizer: heap-use-after-free include/pybind11/detail/smart_holder_poc.h:68:13 in void pybindit::memory::guarded_builtin_delete<int>::operator()<int, 0>(int*)
SUMMARY: AddressSanitizer: heap-use-after-free include/pybind11/detail/smart_holder_poc.h:86:13 in pybindit::memory::guarded_custom_deleter<int, helpers::functor_builtin_delete<int> >::operator()(int*)

With the changes to smart_holder_poc.h the AddressSanitizer errors disappear.

For the record, commands used for ASAN testing (64-bit Debian-like system):

clang++ -fsanitize=address -std=c++11 -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wnon-virtual-dtor -I./include -I../../clone/Catch2/single_include/catch2 -I/usr/include/python3.8 ./tests/pure_cpp/smart_holder_poc_test.cpp
./a.out

Additionally ASAN-tested in the Google build environment.

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 3, 2021

The CI is green, also with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT (#2879): Merging for easier Google-internal testing. This change will get reviewed internally. I'll open a new PR if there are requests for changes.

@rwgk rwgk merged commit 3a336a2 into pybind:smart_holder Mar 3, 2021
@rwgk rwgk deleted the sh_deleter_flag_fix branch March 3, 2021 01:43
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 3, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Mar 3, 2021
rwgk added a commit to google/clif that referenced this pull request Mar 5, 2021
@rwgk
This CL imports two changes made to the smart_holder code:
c9b19ee 
* `shared_ptr<bool> vptr_deleter_armed_flag_ptr` (instead of `unique_ptr`) (pybind/pybind11#2882).
* Changing all but one `std::runtime_error` to `std::invalid_argument` (pybind/pybind11#2883). — `ValueError` is more intuitive and compatible with existing PyCLIF behavior.

This fixes the lifetime issue explained in the "side note" in the description of cl/360478293, and enables two small changes to third_party/clif that are also included in this CL.

TESTED=TGP (http://tap/OCL:360563362:BASE:360596252:1614754358524:3a38713a)

Import pybind/pybind11 from GitHub.

  - 3a336a2047d069081a62338613a747b3d57f645b shared_ptr<bool> vptr_deleter_armed_flag_ptr (instead of ... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 6a7e9f42fe6301e2aa02d5048d8bf752236053db Changing all but one std::runtime_error to std::invalid_a... by Ralf W. Grosse-Kunstleve <rwgk@google.com>

PiperOrigin-RevId: 360746914
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
@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
No reviews
Assignees
No one assigned
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rwgk @EricCousineau-TRI