From 2ea1b908d87b29bcc6214efd3073b92392d495ff Mon Sep 17 00:00:00 2001 From: James Zern Date: Fri, 1 Oct 2021 13:46:02 -0700 Subject: [PATCH] {vp8,vp9}_set_roi_map: fix validation with INT_MIN previously ranges were checked with abs() whose behavior is undefined with INT_MIN. this fixes a crash when the original value is returned and it later used as and offset into a table. Bug: webm:1742 Change-Id: I345970b75c46699587a4fbc4a059e59277f4c2c8 --- test/encode_api_test.cc | 122 ++++++++++++++++++++++++++++++++++++-- vp8/encoder/onyx_if.c | 18 +++--- vp9/encoder/vp9_encoder.c | 13 ++-- 3 files changed, 134 insertions(+), 19 deletions(-) diff --git a/test/encode_api_test.cc b/test/encode_api_test.cc index 6bd7e593d..dec19b226 100644 --- a/test/encode_api_test.cc +++ b/test/encode_api_test.cc @@ -8,6 +8,9 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include +#include + #include "third_party/googletest/src/include/gtest/gtest.h" #include "./vpx_config.h" @@ -18,6 +21,12 @@ namespace { #define NELEMENTS(x) static_cast(sizeof(x) / sizeof(x[0])) +bool IsVP9(const vpx_codec_iface_t *iface) { + static const char kVP9Name[] = "WebM Project VP9"; + return strncmp(kVP9Name, vpx_codec_iface_name(iface), sizeof(kVP9Name) - 1) == + 0; +} + TEST(EncodeAPI, InvalidParams) { static const vpx_codec_iface_t *kCodecs[] = { #if CONFIG_VP8_ENCODER @@ -184,10 +193,7 @@ TEST(EncodeAPI, MultiResEncode) { } // VP9 should report incapable, VP8 invalid for all configurations. - const char kVP9Name[] = "WebM Project VP9"; - const bool is_vp9 = strncmp(kVP9Name, vpx_codec_iface_name(iface), - sizeof(kVP9Name) - 1) == 0; - EXPECT_EQ(is_vp9 ? VPX_CODEC_INCAPABLE : VPX_CODEC_INVALID_PARAM, + EXPECT_EQ(IsVP9(iface) ? VPX_CODEC_INCAPABLE : VPX_CODEC_INVALID_PARAM, vpx_codec_enc_init_multi(&enc[0], iface, &cfg[0], 2, 0, &dsf[0])); for (int i = 0; i < 2; i++) { @@ -196,4 +202,112 @@ TEST(EncodeAPI, MultiResEncode) { } } +TEST(EncodeAPI, SetRoi) { + static struct { + const vpx_codec_iface_t *iface; + int ctrl_id; + } kCodecs[] = { +#if CONFIG_VP8_ENCODER + { &vpx_codec_vp8_cx_algo, VP8E_SET_ROI_MAP }, +#endif +#if CONFIG_VP9_ENCODER + { &vpx_codec_vp9_cx_algo, VP9E_SET_ROI_MAP }, +#endif + }; + constexpr int kWidth = 64; + constexpr int kHeight = 64; + + for (const auto &codec : kCodecs) { + SCOPED_TRACE(vpx_codec_iface_name(codec.iface)); + vpx_codec_ctx_t enc; + vpx_codec_enc_cfg_t cfg; + + EXPECT_EQ(vpx_codec_enc_config_default(codec.iface, &cfg, 0), VPX_CODEC_OK); + cfg.g_w = kWidth; + cfg.g_h = kHeight; + EXPECT_EQ(vpx_codec_enc_init(&enc, codec.iface, &cfg, 0), VPX_CODEC_OK); + + vpx_roi_map_t roi = {}; + uint8_t roi_map[kWidth * kHeight] = {}; + if (IsVP9(codec.iface)) { + roi.rows = (cfg.g_w + 7) >> 3; + roi.cols = (cfg.g_h + 7) >> 3; + } else { + roi.rows = (cfg.g_w + 15) >> 4; + roi.cols = (cfg.g_h + 15) >> 4; + } + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK); + + roi.roi_map = roi_map; + // VP8 only. This value isn't range checked. + roi.static_threshold[1] = 1000; + roi.static_threshold[2] = INT_MIN; + roi.static_threshold[3] = INT_MAX; + + for (const auto delta : { -63, -1, 0, 1, 63 }) { + for (int i = 0; i < 8; ++i) { + roi.delta_q[i] = delta; + roi.delta_lf[i] = delta; + // VP9 only. + roi.skip[i] ^= 1; + roi.ref_frame[i] = (roi.ref_frame[i] + 1) % 4; + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK); + } + } + + vpx_codec_err_t expected_error; + for (const auto delta : { -64, 64, INT_MIN, INT_MAX }) { + expected_error = VPX_CODEC_INVALID_PARAM; + for (int i = 0; i < 8; ++i) { + roi.delta_q[i] = delta; + // The max segment count for VP8 is 4, the remainder of the entries are + // ignored. + if (i >= 4 && !IsVP9(codec.iface)) expected_error = VPX_CODEC_OK; + + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error) + << "delta_q[" << i << "]: " << delta; + roi.delta_q[i] = 0; + + roi.delta_lf[i] = delta; + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error) + << "delta_lf[" << i << "]: " << delta; + roi.delta_lf[i] = 0; + } + } + + // VP8 should ignore skip[] and ref_frame[] values. + expected_error = + IsVP9(codec.iface) ? VPX_CODEC_INVALID_PARAM : VPX_CODEC_OK; + for (const auto skip : { -2, 2, INT_MIN, INT_MAX }) { + for (int i = 0; i < 8; ++i) { + roi.skip[i] = skip; + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error) + << "skip[" << i << "]: " << skip; + roi.skip[i] = 0; + } + } + + // VP9 allows negative values to be used to disable segmentation. + for (int ref_frame = -3; ref_frame < 0; ++ref_frame) { + for (int i = 0; i < 8; ++i) { + roi.ref_frame[i] = ref_frame; + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK) + << "ref_frame[" << i << "]: " << ref_frame; + roi.ref_frame[i] = 0; + } + } + + for (const auto ref_frame : { 4, INT_MIN, INT_MAX }) { + for (int i = 0; i < 8; ++i) { + roi.ref_frame[i] = ref_frame; + EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error) + << "ref_frame[" << i << "]: " << ref_frame; + roi.ref_frame[i] = 0; + } + } + + EXPECT_EQ(vpx_codec_destroy(&enc), VPX_CODEC_OK); + } +} + } // namespace diff --git a/vp8/encoder/onyx_if.c b/vp8/encoder/onyx_if.c index c6c162347..2b059a1e4 100644 --- a/vp8/encoder/onyx_if.c +++ b/vp8/encoder/onyx_if.c @@ -5318,17 +5318,13 @@ int vp8_set_roimap(VP8_COMP *cpi, unsigned char *map, unsigned int rows, return -1; } - // Range check the delta Q values and convert the external Q range values - // to internal ones. - if ((abs(delta_q[0]) > range) || (abs(delta_q[1]) > range) || - (abs(delta_q[2]) > range) || (abs(delta_q[3]) > range)) { - return -1; - } - - // Range check the delta lf values - if ((abs(delta_lf[0]) > range) || (abs(delta_lf[1]) > range) || - (abs(delta_lf[2]) > range) || (abs(delta_lf[3]) > range)) { - return -1; + for (i = 0; i < MAX_MB_SEGMENTS; ++i) { + // Note abs() alone can't be used as the behavior of abs(INT_MIN) is + // undefined. + if (delta_q[i] > range || delta_q[i] < -range || delta_lf[i] > range || + delta_lf[i] < -range) { + return -1; + } } // Also disable segmentation if no deltas are specified. diff --git a/vp9/encoder/vp9_encoder.c b/vp9/encoder/vp9_encoder.c index 6cd8cb80e..7e80835f6 100644 --- a/vp9/encoder/vp9_encoder.c +++ b/vp9/encoder/vp9_encoder.c @@ -654,10 +654,15 @@ static void init_level_info(Vp9LevelInfo *level_info) { } static int check_seg_range(int seg_data[8], int range) { - return !(abs(seg_data[0]) > range || abs(seg_data[1]) > range || - abs(seg_data[2]) > range || abs(seg_data[3]) > range || - abs(seg_data[4]) > range || abs(seg_data[5]) > range || - abs(seg_data[6]) > range || abs(seg_data[7]) > range); + int i; + for (i = 0; i < 8; ++i) { + // Note abs() alone can't be used as the behavior of abs(INT_MIN) is + // undefined. + if (seg_data[i] > range || seg_data[i] < -range) { + return 0; + } + } + return 1; } VP9_LEVEL vp9_get_level(const Vp9LevelSpec *const level_spec) { -- 2.40.0