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.
diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 27bd053..22eceb0 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -929,31 +929,27 @@
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<T>::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<T>::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<py_type>::value) {
+ py_value = as_unsigned<py_type>(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<T>::value && sizeof(py_type) != sizeof(T) &&
- (py_value < (py_type) std::numeric_limits<T>::min() ||
- py_value > (py_type) std::numeric_limits<T>::max()))) {
+ bool py_err = py_value == (py_type) -1 && PyErr_Occurred();
+ if (py_err || (std::is_integral<T>::value && sizeof(py_type) != sizeof(T) &&
+ (py_value < (py_type) std::numeric_limits<T>::min() ||
+ py_value > (py_type) std::numeric_limits<T>::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<object>(std::is_floating_point<T>::value
diff --git a/include/pybind11/common.h b/include/pybind11/common.h
index af0a8d0..1a5f7fa 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 a5aff66..cc48bbb 100644
--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -931,6 +931,28 @@
}
};
+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 <typename Unsigned>
+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 @@
template <typename T,
detail::enable_if_t<std::is_integral<T>::value, int> = 0>
operator T() const {
- if (sizeof(T) <= sizeof(long)) {
- if (std::is_signed<T>::value)
- return (T) PyLong_AsLong(m_ptr);
- else
- return (T) PyLong_AsUnsignedLong(m_ptr);
- } else {
- if (std::is_signed<T>::value)
- return (T) PYBIND11_LONG_AS_LONGLONG(m_ptr);
- else
- return (T) PYBIND11_LONG_AS_UNSIGNED_LONGLONG(m_ptr);
- }
+ return std::is_unsigned<T>::value
+ ? detail::as_unsigned<T>(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 33fe689..55269ba 100644
--- a/tests/test_builtin_casters.cpp
+++ b/tests/test_builtin_casters.cpp
@@ -72,6 +72,12 @@
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<bool, std::string> input) {
return std::make_pair(input.second, input.first);
diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py
index 59af0ee..a6f9b57 100644
--- a/tests/test_builtin_casters.py
+++ b/tests/test_builtin_casters.py
@@ -143,6 +143,44 @@
"""
+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)