From bca568bfc59f944335510762f47fe4414ea1273e Mon Sep 17 00:00:00 2001 From: kwiberg Date: Mon, 23 May 2016 04:07:00 -0700 Subject: [PATCH] Fix an UBSan error (signed overflow) in saturating addition and subtraction Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are supposed to handle overflow gracefully, and they probably did. But since the overflow handling depended on undefined behavior, a sufficiently smart optimizing compiler would have realized that it could just ignore the possibility of overflow and omit all the overflow handling code, leaving only the unadorned addition or subtraction. Also, the new implementations, unlike the old ones, result in branch-free code (tested with clang 3.9 with -O2). BUG=chromium:601728 Review-Url: https://codereview.webrtc.org/2002523002 Cr-Commit-Position: refs/heads/master@{#12846} --- .../signal_processing/include/spl_inl.h | 50 ++++++++----------- .../signal_processing_unittest.cc | 40 ++++++++------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/webrtc/common_audio/signal_processing/include/spl_inl.h b/webrtc/common_audio/signal_processing/include/spl_inl.h index d3cc6dee6c..3c0a81cf34 100644 --- a/webrtc/common_audio/signal_processing/include/spl_inl.h +++ b/webrtc/common_audio/signal_processing/include/spl_inl.h @@ -35,42 +35,32 @@ static __inline int16_t WebRtcSpl_SatW32ToW16(int32_t value32) { return out16; } -static __inline int32_t WebRtcSpl_AddSatW32(int32_t l_var1, int32_t l_var2) { - int32_t l_sum; +static __inline int32_t WebRtcSpl_AddSatW32(int32_t a, int32_t b) { + // Do the addition in unsigned numbers, since signed overflow is undefined + // behavior. + const int32_t sum = (int32_t)((uint32_t)a + (uint32_t)b); - // Perform long addition - l_sum = l_var1 + l_var2; - - if (l_var1 < 0) { // Check for underflow. - if ((l_var2 < 0) && (l_sum >= 0)) { - l_sum = (int32_t)0x80000000; - } - } else { // Check for overflow. - if ((l_var2 > 0) && (l_sum < 0)) { - l_sum = (int32_t)0x7FFFFFFF; - } + // a + b can't overflow if a and b have different signs. If they have the + // same sign, a + b also has the same sign iff it didn't overflow. + if ((a < 0) == (b < 0) && (a < 0) != (sum < 0)) { + // The direction of the overflow is obvious from the sign of a + b. + return sum < 0 ? INT32_MAX : INT32_MIN; } - - return l_sum; + return sum; } -static __inline int32_t WebRtcSpl_SubSatW32(int32_t l_var1, int32_t l_var2) { - int32_t l_diff; +static __inline int32_t WebRtcSpl_SubSatW32(int32_t a, int32_t b) { + // Do the subtraction in unsigned numbers, since signed overflow is undefined + // behavior. + const int32_t diff = (int32_t)((uint32_t)a - (uint32_t)b); - // Perform subtraction. - l_diff = l_var1 - l_var2; - - if (l_var1 < 0) { // Check for underflow. - if ((l_var2 > 0) && (l_diff > 0)) { - l_diff = (int32_t)0x80000000; - } - } else { // Check for overflow. - if ((l_var2 < 0) && (l_diff < 0)) { - l_diff = (int32_t)0x7FFFFFFF; - } + // a - b can't overflow if a and b have the same sign. If they have different + // signs, a - b has the same sign as a iff it didn't overflow. + if ((a < 0) != (b < 0) && (a < 0) != (diff < 0)) { + // The direction of the overflow is obvious from the sign of a - b. + return diff < 0 ? INT32_MAX : INT32_MIN; } - - return l_diff; + return diff; } static __inline int16_t WebRtcSpl_AddSatW16(int16_t a, int16_t b) { diff --git a/webrtc/common_audio/signal_processing/signal_processing_unittest.cc b/webrtc/common_audio/signal_processing/signal_processing_unittest.cc index 108f459c89..0dc0d37878 100644 --- a/webrtc/common_audio/signal_processing/signal_processing_unittest.cc +++ b/webrtc/common_audio/signal_processing/signal_processing_unittest.cc @@ -8,6 +8,9 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include +#include + #include "testing/gtest/include/gtest/gtest.h" #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h" @@ -118,26 +121,25 @@ TEST_F(SplTest, InlineTest) { EXPECT_EQ(104, WebRtcSpl_AddSatW16(a16, b16)); EXPECT_EQ(138, WebRtcSpl_SubSatW16(a16, b16)); +} - EXPECT_EQ(109410, WebRtcSpl_AddSatW32(a32, b32)); - EXPECT_EQ(112832, WebRtcSpl_SubSatW32(a32, b32)); - - a32 = 0x80000000; - b32 = 0x80000000; - // Cast to signed int to avoid compiler complaint on gtest.h. - EXPECT_EQ(static_cast(0x80000000), WebRtcSpl_AddSatW32(a32, b32)); - a32 = 0x7fffffff; - b32 = 0x7fffffff; - EXPECT_EQ(0x7fffffff, WebRtcSpl_AddSatW32(a32, b32)); - a32 = 0; - b32 = 0x80000000; - EXPECT_EQ(0x7fffffff, WebRtcSpl_SubSatW32(a32, b32)); - a32 = 0x7fffffff; - b32 = 0x80000000; - EXPECT_EQ(0x7fffffff, WebRtcSpl_SubSatW32(a32, b32)); - a32 = 0x80000000; - b32 = 0x7fffffff; - EXPECT_EQ(static_cast(0x80000000), WebRtcSpl_SubSatW32(a32, b32)); +TEST_F(SplTest, AddSubSatW32) { + static constexpr int32_t kAddSubArgs[] = { + INT32_MIN, INT32_MIN + 1, -3, -2, -1, 0, 1, -1, 2, + 3, INT32_MAX - 1, INT32_MAX}; + for (int32_t a : kAddSubArgs) { + for (int32_t b : kAddSubArgs) { + const int64_t sum = std::max( + INT32_MIN, std::min(INT32_MAX, static_cast(a) + b)); + const int64_t diff = std::max( + INT32_MIN, std::min(INT32_MAX, static_cast(a) - b)); + std::ostringstream ss; + ss << a << " +/- " << b << ": sum " << sum << ", diff " << diff; + SCOPED_TRACE(ss.str()); + EXPECT_EQ(sum, WebRtcSpl_AddSatW32(a, b)); + EXPECT_EQ(diff, WebRtcSpl_SubSatW32(a, b)); + } + } } TEST_F(SplTest, MathOperationsTest) {