[1883] make sure the conversion to Py_hash_t by tp_hash is safe and avoids -1.

also constify some variables more.

diff --git a/src/lib/dns/python/name_python.cc b/src/lib/dns/python/name_python.cc
index c24d24d51b1d7f05a2390839778eb058b38580af..277595a49fcbfc62d9119eddf78290f7521bbe97 100644 (file)
--- a/src/lib/dns/python/name_python.cc
+++ b/src/lib/dns/python/name_python.cc
@@ -522,8 +522,9 @@ Name_isWildCard(s_Name* self) {
 
 Py_hash_t
 Name_hash(PyObject* pyself) {
-    s_Name* const self = static_cast<s_Name*>(pyself);
-    return (LabelSequence(*self->cppobj).getHash(false));
+    const s_Name* const self = static_cast<s_Name*>(pyself);
+    return (convertToPyHash<size_t>(
+                LabelSequence(*self->cppobj).getHash(false)));
 }
 
 } // end of unnamed namespace

diff --git a/src/lib/dns/python/pydnspp_common.h b/src/lib/dns/python/pydnspp_common.h
index e9e935977e89f52220ffccd5f5e25ef3b9f17365..1513dd99c1f3456bcd7a8e5f8f727abd9847dc05 100644 (file)
--- a/src/lib/dns/python/pydnspp_common.h
+++ b/src/lib/dns/python/pydnspp_common.h
@@ -17,6 +17,9 @@
 
 #include <Python.h>
 
+#include <boost/static_assert.hpp>
+
+#include <climits>  // for CHAR_BIT
 #include <stdexcept>
 #include <string>
 
@@ -48,6 +51,53 @@ int addClassVariable(PyTypeObject& c, const char* name, PyObject* obj);
 #if PY_MINOR_VERSION < 2
 typedef long Py_hash_t;
 #endif
+
+/// \brief Convert a hash value of arbitrary type to a Python hash value.
+///
+/// This templated function is a convenient wrapper to produce a valid hash
+/// value of type Py_hash_t, which is expected to be used as the return value
+/// of a PyTypeObject.tp_hash implementation.  Py_hash_t is a signed integer
+/// the same size as Py_ssize_t.
+///
+/// The major concern is that -1 means an error in tp_hash and so we need to
+/// ensure that this value is never returned.
+///
+/// If the size of the original hash type is small enough, we convert
+/// the original hash value to a value of Py_hash_t, resetting all higher bits
+/// to 0.  Due to the assumption on the sizes, the conversion to HashvalType
+/// is safe, and (after clearing the higher bits) results in a valid positive
+/// value.
+///
+/// If the size of the input and return types is the same, we clear the
+/// highest bit of the original hash so the resulting value will be in a valid
+/// positive range of Py_hash_t.  If the original type is unsigned, there's
+/// probably no better conversion than this because otherwise the conversion
+/// to Py_hash_t could result in an undefined behavior.  If the original type
+/// is signed, this conversion reduces the effective value range of the
+/// original hash.  If this is not desired, the caller should convert it
+/// by itself (note that -1 should be still avoided).
+///
+/// This function does not support the case where the size of the original
+/// hash type is larger than that of Py_hash_t.
+template <typename HashvalType>
+Py_hash_t
+convertToPyHash(HashvalType val) {
+    BOOST_STATIC_ASSERT(sizeof(HashvalType) <= sizeof(Py_hash_t));
+
+    if (sizeof(HashvalType) < sizeof(Py_hash_t)) {
+        // The original hash type has small enough.  Do trivial conversion.
+        const Py_hash_t mask = ~(static_cast<Py_hash_t>(-1) <<
+                                 (CHAR_BIT * sizeof(HashvalType)));
+        return (static_cast<Py_hash_t>(val) & mask);
+    } else {
+        // Clear the highest bit of the original hash so the conversion is
+        // safe and avoids -1.
+        const HashvalType mask = ~(static_cast<HashvalType>(1) <<
+                                   (CHAR_BIT * sizeof(HashvalType) - 1));
+        BOOST_STATIC_ASSERT(mask != static_cast<HashvalType>(-1));
+        return (val & mask);
+    }
+}
 } // namespace python
 } // namespace dns
 } // namespace isc

diff --git a/src/lib/dns/python/rrclass_python.cc b/src/lib/dns/python/rrclass_python.cc
index b94dc0231cce08a76263b4d468c8f5c247bf9825..a566f4744d68b56bd17b6f9f02e44d88f938401c 100644 (file)
--- a/src/lib/dns/python/rrclass_python.cc
+++ b/src/lib/dns/python/rrclass_python.cc
@@ -267,8 +267,8 @@ PyObject* RRClass_ANY(s_RRClass*) {
 
 Py_hash_t
 RRClass_hash(PyObject* pyself) {
-    s_RRClass* const self = static_cast<s_RRClass*>(pyself);
-    return (self->cppobj->getCode());
+    const s_RRClass* const self = static_cast<s_RRClass*>(pyself);
+    return (convertToPyHash<uint16_t>(self->cppobj->getCode()));
 }
 
 } // end anonymous namespace

diff --git a/src/lib/dns/python/rrtype_python.cc b/src/lib/dns/python/rrtype_python.cc
index bf22e8d374f8666c6d2f0c84d52d65b1314fbf01..97b66d4282cf8bb98a257e9ea13929480bd46fec 100644 (file)
--- a/src/lib/dns/python/rrtype_python.cc
+++ b/src/lib/dns/python/rrtype_python.cc
@@ -371,8 +371,8 @@ RRType_ANY(s_RRType*) {
 
 Py_hash_t
 RRType_hash(PyObject* pyself) {
-    s_RRType* const self = static_cast<s_RRType*>(pyself);
-    return (self->cppobj->getCode());
+    const s_RRType* const self = static_cast<s_RRType*>(pyself);
+    return (convertToPyHash<uint16_t>(self->cppobj->getCode()));
 }
 } // end anonymous namespace