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.

Author: jagerman

include/pybind11/cast.h

@@ -929,31 +929,27 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
                 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

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

include/pybind11/pytypes.h

@@ -931,6 +931,28 @@ class bool_ : public object {
     }
 };
 
+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 @@ class int_ : public object {
     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);
     }
 };
 

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<bool, std::string> input) {
         return std::make_pair(input.second, input.first);

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)