{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.

James Zern committed 5 years ago

Commit fccaa5f1 parent b68877a3 files · +134 −19Browse files

Unified · Split

3 files

test/encode_api_test.cc +118 −4
vp8/encoder/onyx_if.c +7 −11
vp9/encoder/vp9_encoder.c +9 −4

test/encode_api_test.cc+118−4

@@ -8,6 +8,9 @@ · 7 unmodified lines
8	* be found in the AUTHORS file in the root of the source tree.	8	* be found in the AUTHORS file in the root of the source tree.
9	*/	9	*/
10		10
		11	#include <climits>
		12	#include <cstring>
		13
11	#include "third_party/googletest/src/include/gtest/gtest.h"	14	#include "third_party/googletest/src/include/gtest/gtest.h"
12		15
13	#include "./vpx_config.h"	16	#include "./vpx_config.h"
@@ -18,6 +21,12 @@ namespace { · 4 unmodified lines
18		21
19	#define NELEMENTS(x) static_cast<int>(sizeof(x) / sizeof(x[0]))	22	#define NELEMENTS(x) static_cast<int>(sizeof(x) / sizeof(x[0]))
20		23
		24	bool IsVP9(const vpx_codec_iface_t *iface) {
		25	static const char kVP9Name[] = "WebM Project VP9";
		26	return strncmp(kVP9Name, vpx_codec_iface_name(iface), sizeof(kVP9Name) - 1) ==
		27	0;
		28	}
		29
21	TEST(EncodeAPI, InvalidParams) {	30	TEST(EncodeAPI, InvalidParams) {
22	static const vpx_codec_iface_t *kCodecs[] = {	31	static const vpx_codec_iface_t *kCodecs[] = {
23	#if CONFIG_VP8_ENCODER	32	#if CONFIG_VP8_ENCODER
@@ -184,10 +193,7 @@ TEST(EncodeAPI, MultiResEncode) { · 160 unmodified lines
184	}	193	}
185		194
186	// VP9 should report incapable, VP8 invalid for all configurations.	195	// VP9 should report incapable, VP8 invalid for all configurations.
187	const char kVP9Name[] = "WebM Project VP9";	196	EXPECT_EQ(IsVP9(iface) ? VPX_CODEC_INCAPABLE : VPX_CODEC_INVALID_PARAM,
188	const bool is_vp9 = strncmp(kVP9Name, vpx_codec_iface_name(iface),
189	sizeof(kVP9Name) - 1) == 0;
190	EXPECT_EQ(is_vp9 ? VPX_CODEC_INCAPABLE : VPX_CODEC_INVALID_PARAM,
191	vpx_codec_enc_init_multi(&enc[0], iface, &cfg[0], 2, 0, &dsf[0]));	197	vpx_codec_enc_init_multi(&enc[0], iface, &cfg[0], 2, 0, &dsf[0]));
192		198
193	for (int i = 0; i < 2; i++) {	199	for (int i = 0; i < 2; i++) {
@@ -196,4 +202,112 @@ TEST(EncodeAPI, MultiResEncode) { · 2 unmodified lines
196	}	202	}
197	}	203	}
198		204
		205	TEST(EncodeAPI, SetRoi) {
		206	static struct {
		207	const vpx_codec_iface_t *iface;
		208	int ctrl_id;
		209	} kCodecs[] = {
		210	#if CONFIG_VP8_ENCODER
		211	{ &vpx_codec_vp8_cx_algo, VP8E_SET_ROI_MAP },
		212	#endif
		213	#if CONFIG_VP9_ENCODER
		214	{ &vpx_codec_vp9_cx_algo, VP9E_SET_ROI_MAP },
		215	#endif
		216	};
		217	constexpr int kWidth = 64;
		218	constexpr int kHeight = 64;
		219
		220	for (const auto &codec : kCodecs) {
		221	SCOPED_TRACE(vpx_codec_iface_name(codec.iface));
		222	vpx_codec_ctx_t enc;
		223	vpx_codec_enc_cfg_t cfg;
		224
		225	EXPECT_EQ(vpx_codec_enc_config_default(codec.iface, &cfg, 0), VPX_CODEC_OK);
		226	cfg.g_w = kWidth;
		227	cfg.g_h = kHeight;
		228	EXPECT_EQ(vpx_codec_enc_init(&enc, codec.iface, &cfg, 0), VPX_CODEC_OK);
		229
		230	vpx_roi_map_t roi = {};
		231	uint8_t roi_map[kWidth * kHeight] = {};
		232	if (IsVP9(codec.iface)) {
		233	roi.rows = (cfg.g_w + 7) >> 3;
		234	roi.cols = (cfg.g_h + 7) >> 3;
		235	} else {
		236	roi.rows = (cfg.g_w + 15) >> 4;
		237	roi.cols = (cfg.g_h + 15) >> 4;
		238	}
		239	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK);
		240
		241	roi.roi_map = roi_map;
		242	// VP8 only. This value isn't range checked.
		243	roi.static_threshold[1] = 1000;
		244	roi.static_threshold[2] = INT_MIN;
		245	roi.static_threshold[3] = INT_MAX;
		246
		247	for (const auto delta : { -63, -1, 0, 1, 63 }) {
		248	for (int i = 0; i < 8; ++i) {
		249	roi.delta_q[i] = delta;
		250	roi.delta_lf[i] = delta;
		251	// VP9 only.
		252	roi.skip[i] ^= 1;
		253	roi.ref_frame[i] = (roi.ref_frame[i] + 1) % 4;
		254	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK);
		255	}
		256	}
		257
		258	vpx_codec_err_t expected_error;
		259	for (const auto delta : { -64, 64, INT_MIN, INT_MAX }) {
		260	expected_error = VPX_CODEC_INVALID_PARAM;
		261	for (int i = 0; i < 8; ++i) {
		262	roi.delta_q[i] = delta;
		263	// The max segment count for VP8 is 4, the remainder of the entries are
		264	// ignored.
		265	if (i >= 4 && !IsVP9(codec.iface)) expected_error = VPX_CODEC_OK;
		266
		267	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error)
		268	<< "delta_q[" << i << "]: " << delta;
		269	roi.delta_q[i] = 0;
		270
		271	roi.delta_lf[i] = delta;
		272	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error)
		273	<< "delta_lf[" << i << "]: " << delta;
		274	roi.delta_lf[i] = 0;
		275	}
		276	}
		277
		278	// VP8 should ignore skip[] and ref_frame[] values.
		279	expected_error =
		280	IsVP9(codec.iface) ? VPX_CODEC_INVALID_PARAM : VPX_CODEC_OK;
		281	for (const auto skip : { -2, 2, INT_MIN, INT_MAX }) {
		282	for (int i = 0; i < 8; ++i) {
		283	roi.skip[i] = skip;
		284	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error)
		285	<< "skip[" << i << "]: " << skip;
		286	roi.skip[i] = 0;
		287	}
		288	}
		289
		290	// VP9 allows negative values to be used to disable segmentation.
		291	for (int ref_frame = -3; ref_frame < 0; ++ref_frame) {
		292	for (int i = 0; i < 8; ++i) {
		293	roi.ref_frame[i] = ref_frame;
		294	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), VPX_CODEC_OK)
		295	<< "ref_frame[" << i << "]: " << ref_frame;
		296	roi.ref_frame[i] = 0;
		297	}
		298	}
		299
		300	for (const auto ref_frame : { 4, INT_MIN, INT_MAX }) {
		301	for (int i = 0; i < 8; ++i) {
		302	roi.ref_frame[i] = ref_frame;
		303	EXPECT_EQ(vpx_codec_control_(&enc, codec.ctrl_id, &roi), expected_error)
		304	<< "ref_frame[" << i << "]: " << ref_frame;
		305	roi.ref_frame[i] = 0;
		306	}
		307	}
		308
		309	EXPECT_EQ(vpx_codec_destroy(&enc), VPX_CODEC_OK);
		310	}
		311	}
		312
199	} // namespace	313	} // namespace

vp8/encoder/onyx_if.c+7−11

@@ -5320,17 +5320,13 @@ int vp8_set_roimap(VP8_COMP cpi, unsigned char map, unsigned int rows, · 5319 unmodified lines
5320	return -1;	5320	return -1;
5321	}	5321	}
5322		5322
5323	// Range check the delta Q values and convert the external Q range values	5323	for (i = 0; i < MAX_MB_SEGMENTS; ++i) {
5324	// to internal ones.	5324	// Note abs() alone can't be used as the behavior of abs(INT_MIN) is
5325	if ((abs(delta_q[0]) > range) \|\| (abs(delta_q[1]) > range) \|\|	5325	// undefined.
5326	(abs(delta_q[2]) > range) \|\| (abs(delta_q[3]) > range)) {	5326	if (delta_q[i] > range \|\| delta_q[i] < -range \|\| delta_lf[i] > range \|\|
5327	return -1;	5327	delta_lf[i] < -range) {
5328	}	5328	return -1;
5329		5329	}
5330	// Range check the delta lf values
5331	if ((abs(delta_lf[0]) > range) \|\| (abs(delta_lf[1]) > range) \|\|
5332	(abs(delta_lf[2]) > range) \|\| (abs(delta_lf[3]) > range)) {
5333	return -1;
5334	}	5330	}
5335		5331
5336	// Also disable segmentation if no deltas are specified.	5332	// Also disable segmentation if no deltas are specified.

vp9/encoder/vp9_encoder.c+9−4

 }
 static int check_seg_range(int seg_data[8], int range) {
-  return !(abs(seg_data[0]) > range || abs(seg_data[1]) > range ||
+  int i;
-           abs(seg_data[2]) > range || abs(seg_data[3]) > range ||
+  for (i = 0; i < 8; ++i) {
-           abs(seg_data[4]) > range || abs(seg_data[5]) > range ||
+    // Note abs() alone can't be used as the behavior of abs(INT_MIN) is
-           abs(seg_data[6]) > range || abs(seg_data[7]) > range);
+    // undefined.
+    if (seg_data[i] > range || seg_data[i] < -range) {
+      return 0;
+    }
+  }
+  return 1;
 }
 VP9_LEVEL vp9_get_level(const Vp9LevelSpec *const level_spec) {

{vp8,vp9}_set_roi_map: fix validation with INT_MIN

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code