From 3f0328f5b508797ac1d8ce60024f6eedd965114f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89milie=20Feral?= Date: Thu, 30 Nov 2017 13:55:11 +0100 Subject: [PATCH] Add assertions for dangerous bit shift that could trigger undefined behaviour Change-Id: I051ed229d407eafcae1ea4b5fc745176a751e036 --- ion/include/ion/keyboard.h | 5 +++++ ion/src/device/regs/register.h | 7 +++++++ liba/src/aeabi-rt/llsl.c | 7 +++++++ liba/src/aeabi-rt/llsr.c | 7 +++++++ poincare/src/integer.cpp | 7 +++++-- 5 files changed, 31 insertions(+), 2 deletions(-) diff --git a/ion/include/ion/keyboard.h b/ion/include/ion/keyboard.h index 47339cb06..9e09314e9 100644 --- a/ion/include/ion/keyboard.h +++ b/ion/include/ion/keyboard.h @@ -3,6 +3,7 @@ extern "C" { #include +#include } namespace Ion { @@ -37,10 +38,14 @@ public: constexpr State(uint64_t s = 0) : m_bitField(s) {} + /* "Shift behavior is undefined if the right operand is negative, or greater + * than or equal to the length in bits of the promoted left operand" according + * to C++ spec but k is always in [0:52]. */ explicit constexpr State(Key k) : m_bitField((uint64_t)1 << (uint8_t)k) {} inline bool keyDown(Key k) const { + assert((uint8_t)k < 64); return (m_bitField>>(uint8_t)k) & 1; } operator uint64_t() const { return m_bitField; } diff --git a/ion/src/device/regs/register.h b/ion/src/device/regs/register.h index da4acb44d..15637284e 100644 --- a/ion/src/device/regs/register.h +++ b/ion/src/device/regs/register.h @@ -2,6 +2,7 @@ #define REGS_REGISTER_H #include +#include template class Register { @@ -21,13 +22,19 @@ public: m_value = bit_range_set_value(high, low, m_value, value); } T getBitRange(uint8_t high, uint8_t low) volatile { + /* "Shift behavior is undefined if the right operand is negative, or greater + * than or equal to the length in bits of the promoted left operand" according + * to C++ spec. */ + assert(low < 8*sizeof(T)); return (m_value & bit_range_mask(high,low)) >> low; } protected: static constexpr T bit_range_mask(uint8_t high, uint8_t low) { + // Same comment as for getBitRange: we should assert (high-low+1) < 8*sizeof(T) return ((((T)1)<<(high-low+1))-1)< typedef unsigned int uint32_t; long long __aeabi_llsl(long long value, int shift) { uint32_t low = (uint32_t)value << shift; + /* "Shift behavior is undefined if the right operand is negative, or greater + * than or equal to the length in bits of the promoted left operand" according + * to C++ spec. However, arm compiler fill the vacated bits with 0 */ + assert(shift < 32 || low == 0); uint32_t high = ((uint32_t)(value >> 32) << shift); + // Same comment + assert(shift < 32 || high == 0); if (shift < 32) { high |= ((uint32_t)value >> (32 - shift)); } else { diff --git a/liba/src/aeabi-rt/llsr.c b/liba/src/aeabi-rt/llsr.c index 38c200cce..0dd3e2cee 100644 --- a/liba/src/aeabi-rt/llsr.c +++ b/liba/src/aeabi-rt/llsr.c @@ -1,14 +1,21 @@ /* See the "Run-time ABI for the ARM Architecture", Section 4.2 */ +#include typedef unsigned int uint32_t; long long __aeabi_llsr(long long value, int shift) { uint32_t low = ((uint32_t)value >> shift); + /* "Shift behavior is undefined if the right operand is negative, or greater + * than or equal to the length in bits of the promoted left operand" according + * to C++ spec. However, arm compiler fill the vacated bits with 0 */ + assert(shift < 32 || low == 0); if (shift < 32) { low |= ((uint32_t)(value >> 32) << (32 - shift)); } else { low |= ((uint32_t)(value >> 32) >> (shift - 32)); } uint32_t high = (uint32_t)(value >> 32) >> shift; + // Same comment + assert(shift < 32 || high == 0); return ((long long)high << 32) | low; } diff --git a/poincare/src/integer.cpp b/poincare/src/integer.cpp index 28dce696b..72622403e 100644 --- a/poincare/src/integer.cpp +++ b/poincare/src/integer.cpp @@ -515,13 +515,15 @@ T Integer::approximate() const { /* Shift the most significant int to the left of the mantissa. The most * significant 1 will be ignore at the end when inserting the mantissa in * the resulting uint64_t (as required by IEEE754). */ + assert(totalNumberOfBits-numberOfBitsInLastDigit > 0 && totalNumberOfBits-numberOfBitsInLastDigit < 64); // Shift operator behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand mantissa |= ((uint64_t)lastDigit << (totalNumberOfBits-numberOfBitsInLastDigit)); int digitIndex = 2; int numberOfBits = numberOfBitsInLastDigit; /* Complete the mantissa by inserting, from left to right, every digit of the * Integer from the most significant one to the last from. We break when - * the mantissa is complete to avoid undefined right shifting (when the shift - * width is wider than the length of the digit in bits). */ + * the mantissa is complete to avoid undefined right shifting (Shift operator + * behavior is undefined if the right operand is negative, or greater than or + * equal to the length in bits of the promoted left operand). */ while (m_numberOfDigits >= digitIndex) { lastDigit = digit(m_numberOfDigits-digitIndex); numberOfBits += 32; @@ -529,6 +531,7 @@ T Integer::approximate() const { break; } if (totalNumberOfBits > numberOfBits) { + assert(totalNumberOfBits-numberOfBits > 0 && totalNumberOfBits-numberOfBits < 64); mantissa |= ((uint64_t)lastDigit << (totalNumberOfBits-numberOfBits)); } else { mantissa |= ((uint64_t)lastDigit >> (numberOfBits-totalNumberOfBits));