From 40af86107765170ef38e8e6998b714b78df9d7e9 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Wed, 23 Sep 2020 11:50:11 +0200 Subject: [PATCH 1/5] ArrowObject back by null external pointers fail gracefully instead of segfault --- r/src/arrow_cpp11.h | 11 +++++++++-- r/tests/testthat/test-arrow.R | 9 +++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/r/src/arrow_cpp11.h b/r/src/arrow_cpp11.h index 1aa3d582e858..6f4a111dbb38 100644 --- a/r/src/arrow_cpp11.h +++ b/r/src/arrow_cpp11.h @@ -157,8 +157,15 @@ struct ns { template Pointer r6_to_pointer(SEXP self) { - return reinterpret_cast( - R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); + void* p = R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)); + if (p == nullptr) { + SEXP klass = Rf_getAttrib(self, R_ClassSymbol); + std::string first_class(Rf_isNull(klass) ? "ArrowObject" + : CHAR(STRING_ELT(klass, 0))); + + cpp11::stop("Invalid <%s>, external pointer to null", first_class.c_str()); + } + return reinterpret_cast(p); } // T is either std::shared_ptr or std::unique_ptr diff --git a/r/tests/testthat/test-arrow.R b/r/tests/testthat/test-arrow.R index 7ef25ac672e5..1672a95f0529 100644 --- a/r/tests/testthat/test-arrow.R +++ b/r/tests/testthat/test-arrow.R @@ -47,3 +47,12 @@ r_only({ ) }) }) + +test_that("arrow gracefully fails to load objects from other sessions (ARROW-10071)", { + a <- Array$create(1:10) + tf <- tempfile(); on.exit(unlink(tf)) + saveRDS(a, tf) + + b <- readRDS(tf) + expect_error(b$length(), "Invalid ") +}) From e77d98ab1f6aeac6e60d9e366567613166d34089 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Thu, 24 Sep 2020 10:42:46 +0200 Subject: [PATCH 2/5] refinements --- r/src/arrow_cpp11.h | 9 +++++---- r/tests/testthat/test-arrow.R | 4 ++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/r/src/arrow_cpp11.h b/r/src/arrow_cpp11.h index 6f4a111dbb38..357a530e4682 100644 --- a/r/src/arrow_cpp11.h +++ b/r/src/arrow_cpp11.h @@ -157,13 +157,14 @@ struct ns { template Pointer r6_to_pointer(SEXP self) { + if (!Rf_inherits(self, "ArrowObject")) { + std::string type_name = typeid(typename std::remove_pointer::type).name(); + cpp11::stop("Invalid R object for %s, must be an ArrowObject", type_name.c_str()); + } void* p = R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)); if (p == nullptr) { SEXP klass = Rf_getAttrib(self, R_ClassSymbol); - std::string first_class(Rf_isNull(klass) ? "ArrowObject" - : CHAR(STRING_ELT(klass, 0))); - - cpp11::stop("Invalid <%s>, external pointer to null", first_class.c_str()); + cpp11::stop("Invalid <%s>, external pointer to null", CHAR(STRING_ELT(klass, 0))); } return reinterpret_cast(p); } diff --git a/r/tests/testthat/test-arrow.R b/r/tests/testthat/test-arrow.R index 1672a95f0529..f1b70e478c8f 100644 --- a/r/tests/testthat/test-arrow.R +++ b/r/tests/testthat/test-arrow.R @@ -56,3 +56,7 @@ test_that("arrow gracefully fails to load objects from other sessions (ARROW-100 b <- readRDS(tf) expect_error(b$length(), "Invalid ") }) + +test_that("check for an ArrowObject in functions use std::shared_ptr", { + expect_error(Array__length(1), "Invalid R object") +}) From 87c26f7a7e5a393681057923a790f6448ea07817 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Fri, 25 Sep 2020 20:26:49 -0400 Subject: [PATCH 3/5] add nameof utility to replace typeid().name() --- cpp/src/arrow/util/nameof.h | 86 +++++++++++++++++++++++++++++++++++++ r/src/arrow_cpp11.h | 6 ++- 2 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 cpp/src/arrow/util/nameof.h diff --git a/cpp/src/arrow/util/nameof.h b/cpp/src/arrow/util/nameof.h new file mode 100644 index 000000000000..d772914976dd --- /dev/null +++ b/cpp/src/arrow/util/nameof.h @@ -0,0 +1,86 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include + +namespace arrow { +namespace util { +namespace detail { + +#ifdef _MSC_VER +#define ARROW_PRETTY_FUNCTION __FUNCSIG__ +#else +#define ARROW_PRETTY_FUNCTION __PRETTY_FUNCTION__ +#endif + +template +constexpr const char* raw() { + return ARROW_PRETTY_FUNCTION; +} + +template +constexpr size_t raw_sizeof() { + return sizeof(ARROW_PRETTY_FUNCTION); +} + +#undef ARROW_PRETTY_FUNCTION + +constexpr bool starts_with(char const* haystack, char const* needle) { + return needle[0] == '\0' || + (haystack[0] == needle[0] && starts_with(haystack + 1, needle + 1)); +} + +constexpr std::size_t search(char const* haystack, char const* needle) { + return haystack[0] == '\0' || starts_with(haystack, needle) + ? 0 + : search(haystack + 1, needle) + 1; +} + +constexpr auto typename_prefix = search(raw(), "void"); + +template +constexpr size_t struct_class_prefix() { +#ifdef _MSC_VER + return starts_with(raw() + typename_prefix, "struct ") + ? 7 + : starts_with(raw() + typename_prefix, "class ") ? 6 : 0; +#else + return 0; +#endif +} + +template +constexpr size_t typename_length() { + // raw_sizeof() - raw_sizeof() == (length of T's name) - strlen("void") + // (length of T's name) == raw_sizeof() - raw_sizeof() + strlen("void") + return raw_sizeof() - raw_sizeof() + 4; +} + +template +constexpr const char* typename_begin() { + return raw() + struct_class_prefix() + typename_prefix; +} + +} // namespace detail + +template +std::string nameof() { + return {detail::typename_begin(), detail::typename_length()}; +} + +} // namespace util +} // namespace arrow diff --git a/r/src/arrow_cpp11.h b/r/src/arrow_cpp11.h index 357a530e4682..26118c6d4e92 100644 --- a/r/src/arrow_cpp11.h +++ b/r/src/arrow_cpp11.h @@ -16,12 +16,15 @@ // under the License. #include // for strlen +#include #include #include #include #include #undef Free +#include "arrow/util/nameof.h" + namespace cpp11 { template @@ -158,7 +161,8 @@ struct ns { template Pointer r6_to_pointer(SEXP self) { if (!Rf_inherits(self, "ArrowObject")) { - std::string type_name = typeid(typename std::remove_pointer::type).name(); + std::string type_name = + arrow::util::nameof::type>(); cpp11::stop("Invalid R object for %s, must be an ArrowObject", type_name.c_str()); } void* p = R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)); From 06e4ac655368ddc75d87a7b72dece1a4b0a71e1e Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Fri, 25 Sep 2020 20:38:54 -0400 Subject: [PATCH 4/5] de-constexpr --- cpp/src/arrow/util/nameof.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/util/nameof.h b/cpp/src/arrow/util/nameof.h index d772914976dd..a46ac7628695 100644 --- a/cpp/src/arrow/util/nameof.h +++ b/cpp/src/arrow/util/nameof.h @@ -28,12 +28,12 @@ namespace detail { #endif template -constexpr const char* raw() { +const char* raw() { return ARROW_PRETTY_FUNCTION; } template -constexpr size_t raw_sizeof() { +size_t raw_sizeof() { return sizeof(ARROW_PRETTY_FUNCTION); } @@ -44,16 +44,16 @@ constexpr bool starts_with(char const* haystack, char const* needle) { (haystack[0] == needle[0] && starts_with(haystack + 1, needle + 1)); } -constexpr std::size_t search(char const* haystack, char const* needle) { +constexpr size_t search(char const* haystack, char const* needle) { return haystack[0] == '\0' || starts_with(haystack, needle) ? 0 : search(haystack + 1, needle) + 1; } -constexpr auto typename_prefix = search(raw(), "void"); +const size_t typename_prefix = search(raw(), "void"); template -constexpr size_t struct_class_prefix() { +size_t struct_class_prefix() { #ifdef _MSC_VER return starts_with(raw() + typename_prefix, "struct ") ? 7 @@ -64,14 +64,14 @@ constexpr size_t struct_class_prefix() { } template -constexpr size_t typename_length() { +size_t typename_length() { // raw_sizeof() - raw_sizeof() == (length of T's name) - strlen("void") // (length of T's name) == raw_sizeof() - raw_sizeof() + strlen("void") return raw_sizeof() - raw_sizeof() + 4; } template -constexpr const char* typename_begin() { +const char* typename_begin() { return raw() + struct_class_prefix() + typename_prefix; } From 0086446cf608e20936cca2bcd269c4194e192053 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Sat, 26 Sep 2020 15:59:53 -0400 Subject: [PATCH 5/5] remove include --- r/src/arrow_cpp11.h | 1 - 1 file changed, 1 deletion(-) diff --git a/r/src/arrow_cpp11.h b/r/src/arrow_cpp11.h index 26118c6d4e92..f5d0cbc1a9c9 100644 --- a/r/src/arrow_cpp11.h +++ b/r/src/arrow_cpp11.h @@ -16,7 +16,6 @@ // under the License. #include // for strlen -#include #include #include #include