pybind / pybind/pybind11

[BUG] Keep pybind11 type casters alive recursively when needed

Open
#2,527 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
18k
Forks
2.3k
Avg merge
5d 17h
Merged PRs (30d)
10

Description

Issue description

When pybind11 casts from python->C++ for a function call, the casters for each argument are kept alive for the duration of the function call. This is important when passing a pointer or reference to memory owned by the caster (rather than owned by python). std::string is a common example of this- functions which take a std::string* or std::string& as one of their arguments (const or non-const) would fail without this behavior because the cast string would be freed before the function begins executing.

However, this behavior does not work recursively. For example, the list_caster constructs a caster for each element, moves the result out of it, and then discards the caster: https://github.com/pybind/pybind11/blob/3c7ef56bb651c4d31670c0db42d9555755b1172a/include/pybind11/stl.h#L152-L157

Thus, if you attempt to bind a function which takes a std::vector<std::string*> as one of its arguments, it will fail (and probably corrupt memory) because the std::string is owned by the element caster, which is destroyed before the function call begins.

This is particularly a problem when writing a caster for Spans (https://abseil.io/tips/93). Since the span does not confer ownership, the caster must retain ownership of a vector for the span to reference. This breaks with nested spans (Span<const Span<const T>>) exactly the same way std::vector<std::string*> breaks.

However, just keeping the casters alive recursively would be unnecessary and inefficient in many cases. For efficiency, I think the element casters should only be kept alive recursively if both of the following are true:

  1. The element caster owns the C++ representation.
  2. The element can't be moved out of the caster because moving the C++ type is not possible or does not also transfer ownership, such as with raw pointers.

The second condition is easy enough check- if the element type is a pointer, l-value reference, or the caster does not support the movable_cast_op_type, then it is satisfied. This can be checked by adding a r-value reference to the element type and feeding it through the element caster's cast_op_type:

using ElementCaster = make_caster<ElementType>;
static constexpr bool kCondition2 =
  !std::is_rvalue_reference_v<typename ElementCaster::template cast_op_type<
     typename std::add_rvalue_reference_t<ElementType>>>;

However, there does not appear to be a way to check the the first condition; type_casters would probably have to declare it, which would require an addition to the type_caster API.

Reproducible example code

C++

    // `std::vector<std::string*> does not work because the string is owned by
    // the element caster, which is destroyed before the function is called.
    m.def("load_vector_str_ptr", [](const std::vector<std::string*>& v) {
        // print more reliably triggers a memory error, making the use-after-free more obvious.
        py::print(*v.at(0), " && ", *v.at(1));
        return *v.at(0) == "test1" && *v.at(1) == "test2";
    });

python

assert m.load_vector_str_ptr(["test1", "test2"])

I'll upload a PR as soon as I figure out how...

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reproducing the use-after-free example with the recursive caster code in include/pybind11/stl.h around lines 152-157. Trace how element casters are created and discarded, then add coverage showing that vector<string*> and nested span-like casters remain valid through the bound function call; done means the regression reproducer succeeds without invalid memory use.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.