From 259b2fafea69c546111f1eab814e46afed17a1e6 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sat, 1 Jul 2017 16:31:49 -0400 Subject: [PATCH] Fix unsigned error value casting When casting to an unsigned type from a python 2 `int`, we currently cast using `(unsigned long long) PyLong_AsUnsignedLong(src.ptr())`. If the Python cast fails, it returns (unsigned long) -1, but then we cast this to `unsigned long long`, which means we get 4294967295, but because that isn't equal to `(unsigned long long) -1`, we don't detect the failure. This commit moves the unsigned casting into a `detail::as_unsigned` function which, upon error, casts -1 to the final type, and otherwise casts the return value to the final type to avoid the problematic double cast when an error occurs. The error most commonly shows up wherever `long` is 32-bits (e.g. under both 32- and 64-bit Windows, and under 32-bit linux) when passing a negative value to a bound function taking an `unsigned long`. Fixes #929. The added tests also trigger a latent segfault under PyPy: when casting to an integer smaller than `long` (e.g. casting to a `uint32_t` on a 64-bit `long` architecture) we check both for a Python error and also that the resulting intermediate value will fit in the final type. If there is no conversion error, but we get a value that would overflow, we end up calling `PyErr_ExceptionMatches()` illegally: that call is only allowed when there is a current exception. Under PyPy, this segfaults the test suite. It doesn't appear to segfault under CPython, but the documentation suggests that it *could* do so. The fix is to only check for the exception match if we actually got an error. --- include/pybind11/cast.h | 36 ++++++++++++++------------------ include/pybind11/common.h | 2 -- include/pybind11/pytypes.h | 38 ++++++++++++++++++++++++---------- tests/test_builtin_casters.cpp | 6 ++++++ tests/test_builtin_casters.py | 38 ++++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 33 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 27bd05359..22eceb05f 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -929,31 +929,27 @@ public: py_value = (py_type) PyFloat_AsDouble(src.ptr()); else return false; - } else if (sizeof(T) <= sizeof(long)) { - if (PyFloat_Check(src.ptr())) - return false; - if (std::is_signed::value) - py_value = (py_type) PyLong_AsLong(src.ptr()); - else - py_value = (py_type) PyLong_AsUnsignedLong(src.ptr()); - } else { - if (PyFloat_Check(src.ptr())) - return false; - if (std::is_signed::value) - py_value = (py_type) PYBIND11_LONG_AS_LONGLONG(src.ptr()); - else - py_value = (py_type) PYBIND11_LONG_AS_UNSIGNED_LONGLONG(src.ptr()); + } else if (PyFloat_Check(src.ptr())) { + return false; + } else if (std::is_unsigned::value) { + py_value = as_unsigned(src.ptr()); + } else { // signed integer: + py_value = sizeof(T) <= sizeof(long) + ? (py_type) PyLong_AsLong(src.ptr()) + : (py_type) PYBIND11_LONG_AS_LONGLONG(src.ptr()); } - if ((py_value == (py_type) -1 && PyErr_Occurred()) || - (std::is_integral::value && sizeof(py_type) != sizeof(T) && - (py_value < (py_type) std::numeric_limits::min() || - py_value > (py_type) std::numeric_limits::max()))) { + bool py_err = py_value == (py_type) -1 && PyErr_Occurred(); + if (py_err || (std::is_integral::value && sizeof(py_type) != sizeof(T) && + (py_value < (py_type) std::numeric_limits::min() || + py_value > (py_type) std::numeric_limits::max()))) { + bool type_error = py_err && PyErr_ExceptionMatches( #if PY_VERSION_HEX < 0x03000000 && !defined(PYPY_VERSION) - bool type_error = PyErr_ExceptionMatches(PyExc_SystemError); + PyExc_SystemError #else - bool type_error = PyErr_ExceptionMatches(PyExc_TypeError); + PyExc_TypeError #endif + ); PyErr_Clear(); if (type_error && convert && PyNumber_Check(src.ptr())) { auto tmp = reinterpret_borrow(std::is_floating_point::value diff --git a/include/pybind11/common.h b/include/pybind11/common.h index af0a8d0e3..1a5f7fa4e 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -147,7 +147,6 @@ #define PYBIND11_BYTES_SIZE PyBytes_Size #define PYBIND11_LONG_CHECK(o) PyLong_Check(o) #define PYBIND11_LONG_AS_LONGLONG(o) PyLong_AsLongLong(o) -#define PYBIND11_LONG_AS_UNSIGNED_LONGLONG(o) PyLong_AsUnsignedLongLong(o) #define PYBIND11_BYTES_NAME "bytes" #define PYBIND11_STRING_NAME "str" #define PYBIND11_SLICE_OBJECT PyObject @@ -167,7 +166,6 @@ #define PYBIND11_BYTES_SIZE PyString_Size #define PYBIND11_LONG_CHECK(o) (PyInt_Check(o) || PyLong_Check(o)) #define PYBIND11_LONG_AS_LONGLONG(o) (PyInt_Check(o) ? (long long) PyLong_AsLong(o) : PyLong_AsLongLong(o)) -#define PYBIND11_LONG_AS_UNSIGNED_LONGLONG(o) (PyInt_Check(o) ? (unsigned long long) PyLong_AsUnsignedLong(o) : PyLong_AsUnsignedLongLong(o)) #define PYBIND11_BYTES_NAME "str" #define PYBIND11_STRING_NAME "unicode" #define PYBIND11_SLICE_OBJECT PySliceObject diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index a5aff662a..cc48bbbff 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -931,6 +931,28 @@ private: } }; +NAMESPACE_BEGIN(detail) +// Converts a value to the given unsigned type. If an error occurs, you get back (Unsigned) -1; +// otherwise you get back the unsigned long or unsigned long long value cast to (Unsigned). +// (The distinction is critically important when casting a returned -1 error value to some other +// unsigned type: (A)-1 != (B)-1 when A and B are unsigned types of different sizes). +template +Unsigned as_unsigned(PyObject *o) { + if (sizeof(Unsigned) <= sizeof(unsigned long) +#if PY_VERSION_HEX < 0x03000000 + || PyInt_Check(o) +#endif + ) { + unsigned long v = PyLong_AsUnsignedLong(o); + return v == (unsigned long) -1 && PyErr_Occurred() ? (Unsigned) -1 : (Unsigned) v; + } + else { + unsigned long long v = PyLong_AsUnsignedLongLong(o); + return v == (unsigned long long) -1 && PyErr_Occurred() ? (Unsigned) -1 : (Unsigned) v; + } +} +NAMESPACE_END(detail) + class int_ : public object { public: PYBIND11_OBJECT_CVT(int_, object, PYBIND11_LONG_CHECK, PyNumber_Long) @@ -956,17 +978,11 @@ public: template ::value, int> = 0> operator T() const { - if (sizeof(T) <= sizeof(long)) { - if (std::is_signed::value) - return (T) PyLong_AsLong(m_ptr); - else - return (T) PyLong_AsUnsignedLong(m_ptr); - } else { - if (std::is_signed::value) - return (T) PYBIND11_LONG_AS_LONGLONG(m_ptr); - else - return (T) PYBIND11_LONG_AS_UNSIGNED_LONGLONG(m_ptr); - } + return std::is_unsigned::value + ? detail::as_unsigned(m_ptr) + : sizeof(T) <= sizeof(long) + ? (T) PyLong_AsLong(m_ptr) + : (T) PYBIND11_LONG_AS_LONGLONG(m_ptr); } }; diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 33fe689a0..55269bac6 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -72,6 +72,12 @@ TEST_SUBMODULE(builtin_casters, m) { m.def("string_view32_return", []() { return std::u32string_view(U"utf32 secret \U0001f382"); }); #endif + // test_integer_casting + m.def("i32_str", [](std::int32_t v) { return std::to_string(v); }); + m.def("u32_str", [](std::uint32_t v) { return std::to_string(v); }); + m.def("i64_str", [](std::int64_t v) { return std::to_string(v); }); + m.def("u64_str", [](std::uint64_t v) { return std::to_string(v); }); + // test_tuple m.def("pair_passthrough", [](std::pair input) { return std::make_pair(input.second, input.first); diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index 59af0ee9a..a6f9b57af 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -143,6 +143,44 @@ def test_string_view(capture): """ +def test_integer_casting(): + """Issue #929 - out-of-range integer values shouldn't be accepted""" + import sys + assert m.i32_str(-1) == "-1" + assert m.i64_str(-1) == "-1" + assert m.i32_str(2000000000) == "2000000000" + assert m.u32_str(2000000000) == "2000000000" + if sys.version_info < (3,): + assert m.i32_str(long(-1)) == "-1" + assert m.i64_str(long(-1)) == "-1" + assert m.i64_str(long(-999999999999)) == "-999999999999" + assert m.u64_str(long(999999999999)) == "999999999999" + else: + assert m.i64_str(-999999999999) == "-999999999999" + assert m.u64_str(999999999999) == "999999999999" + + with pytest.raises(TypeError) as excinfo: + m.u32_str(-1) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.u64_str(-1) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.i32_str(-3000000000) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.i32_str(3000000000) + assert "incompatible function arguments" in str(excinfo.value) + + if sys.version_info < (3,): + with pytest.raises(TypeError) as excinfo: + m.u32_str(long(-1)) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.u64_str(long(-1)) + assert "incompatible function arguments" in str(excinfo.value) + + def test_tuple(doc): """std::pair <-> tuple & std::tuple <-> tuple""" assert m.pair_passthrough((True, "test")) == ("test", True)