fix: Avoid premature release for external pointers - #2
Open
krlmlr wants to merge 12 commits into
Open
Conversation
* Don't rely on transitive include of `"fmt/core.h"` We got it through `cpp11/protect.hpp`, but we should not rely on that * Correctly use `fmt::runtime()` on runtime strings passed to `fmt::format()` In fmt, there is a `FMT_CONSTEVAL` macro that resolves to `consteval` on "new enough" C++ (otherwise it doesn't do anything). For R 4.6+, the default C++ used is finally "new enough" (`__cplusplus > 201703L`). This causes all `fmt::format()` calls to require a constant expression for `const char*` and `std::string&` input, which we are not currently doing via `fmt_arg`. We have a runtime provided string, which must now be wrapped in `fmt::runtime()`, which is what we should have been doing all along. * Add `r_ns_env()` and use in `get_namespace()` Throwing an informative (and tested!) error when we can't find the package namespace * Define `RCPP_NO_R_HEADERS_CHECK` before all `#include <Rcpp.h>` usage `#include <cpp11/R.hpp>` sets everything up the right way, and otherwise we get a warning from Rcpp which doesn't seem to end up being relevant for this use case * Use `r_env_has()` + `r_env_get()` To avoid triggering a NOTE about usage of `Rf_findVarInFrame3()` on R 4.5, where technically we had the tools to avoid that
* Teach `cpp_source()` how to source multiple files * NEWS bullet * Link to PR
…violations (r-lib#493) * Add failing test * Use tag types to generate attribute specific internals * NEWS bullet * Formatting
* Update `cran-comments.md` * Increment version number to 0.5.5
The move constructor and move assignment operator went through `reset(rhs.release())`, which dropped the SEXP's attributes and released the pointer before the new owner had taken it over. Move the `data_` member directly instead, and clear the right-hand side afterwards. Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Patch
b-extptr-leakof the stack.external_pointer's move constructor and move assignment operator both went throughreset(rhs.release()).release()hands back the raw pointer and drops theSEXPthat carried it, so the attributes on thatSEXPwere lost across a move, andreset()then built a fresh external pointer around the raw address — releasing the old one before the new owner had taken it over.Moving the
data_member directly keeps theSEXPitself, attributes and all, and clearing the right-hand side afterwards is what makes it a move.data_is acpp11::sexp, which owns the protection, so this is correct even for self-move.The move assignment operator was also missing its
return *this;— it was declared to return a reference and fell off the end.Tests in
cpp11test/src/test-external_pointer.cppcover attribute preservation through both the move constructor and move assignment.Verification
cpp11test'sexternal_pointer-C++suite passes, as does the rest of the C++ suite, against an install of this branch.Squashed from five commits on the original
b-extptr-leak, three of which were "Remove", "Clarify" and "Formatting". The original branch is untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01K8MneV8KqHYUuC8fWV3X5Q
Generated by Claude Code