From 0dec0897f1834f3d6fd8b6bfbb59de5e42c27459 Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 17 Dec 2024 14:35:08 +0100 Subject: [PATCH] Make I420DataSize trigger a crash in case of int overflow. Bug: chromium:371686447 Fixes: chromium:371686447 Change-Id: Icd4ef5f1edc54853445bb1542eff62e354655368 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371900 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#43588} --- api/video/BUILD.gn | 1 + api/video/i420_buffer.cc | 15 +++++++++++++-- common_video/libyuv/libyuv_unittest.cc | 24 ++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/api/video/BUILD.gn b/api/video/BUILD.gn index 57e4d9f97e..337e02f61c 100644 --- a/api/video/BUILD.gn +++ b/api/video/BUILD.gn @@ -69,6 +69,7 @@ rtc_library("video_frame") { "..:video_track_source_constraints", "../../rtc_base:checks", "../../rtc_base:refcount", + "../../rtc_base:safe_conversions", "../../rtc_base:timeutils", "../../rtc_base/memory:aligned_malloc", "../../rtc_base/system:rtc_export", diff --git a/api/video/i420_buffer.cc b/api/video/i420_buffer.cc index c2ddd26a27..5a57ee08b9 100644 --- a/api/video/i420_buffer.cc +++ b/api/video/i420_buffer.cc @@ -13,6 +13,7 @@ #include #include +#include #include #include "api/make_ref_counted.h" @@ -21,6 +22,7 @@ #include "api/video/video_rotation.h" #include "rtc_base/checks.h" #include "rtc_base/memory/aligned_malloc.h" +#include "rtc_base/numerics/safe_conversions.h" #include "third_party/libyuv/include/libyuv/convert.h" #include "third_party/libyuv/include/libyuv/planar_functions.h" #include "third_party/libyuv/include/libyuv/rotate.h" @@ -33,8 +35,17 @@ namespace webrtc { namespace { -int I420DataSize(int height, int stride_y, int stride_u, int stride_v) { - return stride_y * height + (stride_u + stride_v) * ((height + 1) / 2); +// Do the size calculation using 64bit integers and check for int overflow. +int I420DataSize(int64_t height, + int64_t stride_y, + int64_t stride_u, + int64_t stride_v) { + RTC_DCHECK(height >= 0 && height <= std::numeric_limits::max()); + RTC_DCHECK(stride_y >= 0 && stride_y <= std::numeric_limits::max()); + RTC_DCHECK(stride_u >= 0 && stride_u <= std::numeric_limits::max()); + RTC_DCHECK(stride_v >= 0 && stride_v <= std::numeric_limits::max()); + return rtc::checked_cast(stride_y * height + + (stride_u + stride_v) * ((height + 1) / 2)); } } // namespace diff --git a/common_video/libyuv/libyuv_unittest.cc b/common_video/libyuv/libyuv_unittest.cc index f9c82f6284..6d569c0e39 100644 --- a/common_video/libyuv/libyuv_unittest.cc +++ b/common_video/libyuv/libyuv_unittest.cc @@ -18,6 +18,7 @@ #include "api/video/i420_buffer.h" #include "api/video/video_frame.h" #include "common_video/libyuv/include/webrtc_libyuv.h" +#include "rtc_base/logging.h" #include "test/frame_utils.h" #include "test/gmock.h" #include "test/gtest.h" @@ -383,4 +384,27 @@ TEST(I420WeightedPSNRTest, SmokeTest) { /*abs_error=*/0.001); } +#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +// Check that we catch int overflow if invalid dimensions get passed to +// `I420Buffer::Create()`. +TEST_F(TestLibYuv, I420DimensionsTooLarge) { + // Dimensions large enough to cause overflow. + constexpr int kWidth = 0xFFFF; + constexpr int kHeight = 0xAAB0; + // Sanity check for this test. This calculation, which is part of what + // `I420Buffer::Create()` will do, should cause an `int` overflow. + static_assert( + (int64_t{kWidth} * int64_t{kHeight}) > std::numeric_limits::max(), + ""); + + // The Y plane is the image width * height, while the strides of the U and V + // planes are half the width. + const int stride_uv = (kWidth + 1) / 2; + EXPECT_DEATH(I420Buffer::Create(kWidth, kHeight, /*stride_y=*/kWidth, + /*stride_u=*/stride_uv, + /*stride_v=*/stride_uv), + "IsValueInRangeForNumericType"); +} +#endif // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) + } // namespace webrtc