From abd0a7c8c420d099ce4facdf3ac3085bd8f62243 Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Thu, 4 Jun 2020 10:04:54 -0400 Subject: [PATCH 1/4] Replace error suppression with static assertion Rather than suppressing the unnecessary compiler warning, we will avoid the question by removing the lower bound and introducing a static assertion about the type of `day`. In some ways this is still worse than the original version, since it only does the assertion when the __typeof__ GCC extension is available, and if the type of `day` is changed to be signed it will actually require code changes to the boundary check, but for practical purposes (since, at least at the moment, there's very little chance that we will remove GCC-based compilations from our CI checks) this will at least prevent the introduction of this particular bug. https://bugs.python.org/issue40686 --- Modules/_zoneinfo.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/Modules/_zoneinfo.c b/Modules/_zoneinfo.c index bee59b8d2ae0cc..6ec999c1b1d8d0 100644 --- a/Modules/_zoneinfo.c +++ b/Modules/_zoneinfo.c @@ -7,6 +7,17 @@ #include "datetime.h" +// This takes advantage of GCC compiler extensions to determine the type of the +// variable, so we'll make Py_ASSERT_VAR_UNSIGNED a noop on non-GCC platforms. +// This is not perfect, but it's better than nothing. +#ifdef __GNUC__ +#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0) +#define Py_ASSERT_VAR_UNSIGNED(var) \ + Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(__typeof__(var))) +#else +#define Py_ASSERT_VAR_UNSIGNED(var) +#endif + // Imports static PyObject *io_open = NULL; static PyObject *_tzpath_find_tzfile = NULL; @@ -1217,15 +1228,12 @@ calendarrule_new(uint8_t month, uint8_t week, uint8_t day, int8_t hour, return -1; } - // day is an unsigned integer, so day < 0 should always return false, but - // if day's type changes to a signed integer *without* changing this value, - // it may create a bug. Considering that the compiler should be able to - // optimize out the first comparison if day is an unsigned integer anyway, - // we will leave this comparison in place and disable the compiler warning. -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wtype-limits" - if (day < 0 || day > 6) { -#pragma GCC diagnostic pop + // The actual bounds of day are (day >= 0 && day <= 6), but since day is an + // unsigned variable, day >= 0 is always true. To ensure that a bug is not + // introduced in the event that someone changes day to a signed type, we + // will assert that it is an unsigned type. + Py_ASSERT_VAR_UNSIGNED(day); + if (day > 6) { PyErr_Format(PyExc_ValueError, "Day must be in [0, 6]"); return -1; } From cec55efa0e5557b88274c8642a87a83ffbd4493d Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Thu, 4 Jun 2020 13:45:18 -0400 Subject: [PATCH 2/4] Add macros to get type a variable's type --- Include/pymacro.h | 1 + Include/pyport.h | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/Include/pymacro.h b/Include/pymacro.h index 202b936d964f00..abb2f5f7489608 100644 --- a/Include/pymacro.h +++ b/Include/pymacro.h @@ -88,6 +88,7 @@ (uintptr_t)((a) - 1)) & ~(uintptr_t)((a) - 1))) /* Check if pointer "p" is aligned to "a"-bytes boundary. */ #define _Py_IS_ALIGNED(p, a) (!((uintptr_t)(p) & (uintptr_t)((a) - 1))) +#define _Py_IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0) /* Use this for unused arguments in a function definition to silence compiler * warnings. Example: diff --git a/Include/pyport.h b/Include/pyport.h index 7137006870bf01..26c745f3507ae8 100644 --- a/Include/pyport.h +++ b/Include/pyport.h @@ -162,6 +162,27 @@ typedef int Py_ssize_clean_t; # define PY_FORMAT_SIZE_T "z" #endif + +/* _Py_HAVE_TYPEOF and _Py_TYPEOF can opportunistically support an equivalent + * of GCC's typeof extension where possible. It is not possible to get + * equivalent behavior on all platforms, so all uses of _Py_TYPEOF should be + * guarded by _Py_HAVE_TYPEOF. + */ +#if defined(__GNUC__) || defined(__clang__) || defined(__cplusplus) +# define _Py_HAVE_TYPEOF 1 +# if defined(__cplusplus) +# define _Py_TYPEOF(x) decltype(x) +# else +# define _Py_TYPEOF(x) __typeof__(x) +# endif +#else +# define _Py_TYPEOF(x) Py_FatalError( \ + "_Py_TYPEOF is not available in all supported compilation modes on " \ + "all supported compilers. Use the _Py_HAVE_TYPEOF macro to guard " \ + "any statements using _Py_TYPEOF." \ + ) +#endif + /* Py_LOCAL can be used instead of static to get the fastest possible calling * convention for functions that are local to a given module. * From 93a13c343ef859650a6883bb0d17c3cc7b2e9f61 Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Thu, 4 Jun 2020 13:45:42 -0400 Subject: [PATCH 3/4] Use _Py_TYPEOF in _zoneinfo.c --- Modules/_zoneinfo.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Modules/_zoneinfo.c b/Modules/_zoneinfo.c index 6ec999c1b1d8d0..48bb0a9e618ee0 100644 --- a/Modules/_zoneinfo.c +++ b/Modules/_zoneinfo.c @@ -10,10 +10,9 @@ // This takes advantage of GCC compiler extensions to determine the type of the // variable, so we'll make Py_ASSERT_VAR_UNSIGNED a noop on non-GCC platforms. // This is not perfect, but it's better than nothing. -#ifdef __GNUC__ -#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0) +#ifdef _Py_HAVE_TYPEOF #define Py_ASSERT_VAR_UNSIGNED(var) \ - Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(__typeof__(var))) + Py_BUILD_ASSERT(_Py_IS_TYPE_UNSIGNED(_Py_TYPEOF(var))) #else #define Py_ASSERT_VAR_UNSIGNED(var) #endif From b948cb23ff7fe6c5e3a51882ac4f3771651ccbfe Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Tue, 20 Oct 2020 14:11:38 -0400 Subject: [PATCH 4/4] Revert "Use _Py_TYPEOF in _zoneinfo.c" This reverts commit 93a13c343ef859650a6883bb0d17c3cc7b2e9f61. --- Modules/_zoneinfo.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/_zoneinfo.c b/Modules/_zoneinfo.c index 48bb0a9e618ee0..6ec999c1b1d8d0 100644 --- a/Modules/_zoneinfo.c +++ b/Modules/_zoneinfo.c @@ -10,9 +10,10 @@ // This takes advantage of GCC compiler extensions to determine the type of the // variable, so we'll make Py_ASSERT_VAR_UNSIGNED a noop on non-GCC platforms. // This is not perfect, but it's better than nothing. -#ifdef _Py_HAVE_TYPEOF +#ifdef __GNUC__ +#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0) #define Py_ASSERT_VAR_UNSIGNED(var) \ - Py_BUILD_ASSERT(_Py_IS_TYPE_UNSIGNED(_Py_TYPEOF(var))) + Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(__typeof__(var))) #else #define Py_ASSERT_VAR_UNSIGNED(var) #endif