Skip to content

Commit

Permalink
vp8: fix OOB access in x->MVcount
Browse files Browse the repository at this point in the history 
Motion vectors are now clamped in
vp8_find_best_sub_pixel_step_iteratively, vp8_find_best_sub_pixel_step,
vp8_find_best_half_pixel_step, vp8_full_search_sad,
vp8_refining_search_sadx4 and vp8_refining_search_sad_c (the rtcd for
other optimizations are redirects to vp8_refining_search_sadx4).

The difference of valid motion vectors may still go beyond the range of
the MVcount array, however, so additional checks are added to
rd_update_mvcount() and update_mvcount().

Note the test source and settings (speed 1 and GOOD quality mode) come
from the issue report; additional coverage is added for realtime. The
realtime path does not trigger the error without the fix, but as it's
similar to the rd path, the same clamp is done to be safe.

Fixes:
vp8/encoder/rdopt.c:1579:5: runtime error: index 17467 out of bounds for
  type 'unsigned int[2047]'

Bug: oss-fuzz:69906
Change-Id: Ia8bd087cfe4475ab09ba711ed806fbcbaa72e552
  • Loading branch information
@jzern
jzern committed Jul 25, 2024
1 parent f9120b7 commit cdf8da4
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 38 deletions.
58 changes: 58 additions & 0 deletions test/encode_api_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "third_party/googletest/src/include/gtest/gtest.h"
#include "test/acm_random.h"
#include "test/video_source.h"
#include "test/y4m_video_source.h"

#include "./vpx_config.h"
#include "vpx/vp8cx.h"
Expand Down Expand Up @@ -593,6 +594,63 @@ TEST(EncodeAPI, OssFuzz69100) {

ASSERT_EQ(vpx_codec_destroy(&enc), VPX_CODEC_OK);
}

void EncodeOssFuzz69906(int cpu_used, vpx_enc_deadline_t deadline) {
char str[80];
snprintf(str, sizeof(str), "cpu_used: %d deadline: %d", cpu_used,
static_cast<int>(deadline));
SCOPED_TRACE(str);

// Initialize libvpx encoder.
vpx_codec_iface_t *const iface = vpx_codec_vp8_cx();
vpx_codec_ctx_t enc;
vpx_codec_enc_cfg_t cfg;

ASSERT_EQ(vpx_codec_enc_config_default(iface, &cfg, 0), VPX_CODEC_OK);

cfg.g_w = 4097;
cfg.g_h = 16;
cfg.rc_target_bitrate = 1237084865;
cfg.kf_max_dist = 4336;

ASSERT_EQ(vpx_codec_enc_init(&enc, iface, &cfg, 0), VPX_CODEC_OK);

ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_CPUUSED, cpu_used), VPX_CODEC_OK);
ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_ARNR_MAXFRAMES, 0), VPX_CODEC_OK);
ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_ARNR_STRENGTH, 3), VPX_CODEC_OK);
ASSERT_EQ(vpx_codec_control_(&enc, VP8E_SET_ARNR_TYPE, 3),
VPX_CODEC_OK); // deprecated
ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_NOISE_SENSITIVITY, 0),
VPX_CODEC_OK);
ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_TOKEN_PARTITIONS, 0),
VPX_CODEC_OK);
ASSERT_EQ(vpx_codec_control(&enc, VP8E_SET_STATIC_THRESHOLD, 0),
VPX_CODEC_OK);

libvpx_test::Y4mVideoSource video("repro-oss-fuzz-69906.y4m", /*start=*/0,
/*limit=*/3);
video.Begin();
do {
ASSERT_EQ(vpx_codec_encode(&enc, video.img(), video.pts(), video.duration(),
/*flags=*/0, deadline),
VPX_CODEC_OK);
video.Next();
} while (video.img() != nullptr);

ASSERT_EQ(vpx_codec_destroy(&enc), VPX_CODEC_OK);
}

TEST(EncodeAPI, OssFuzz69906) {
// Note the original bug report was for speed 1, good quality. The remainder
// of the settings are for added coverage.
for (int cpu_used = 0; cpu_used <= 5; ++cpu_used) {
EncodeOssFuzz69906(cpu_used, VPX_DL_GOOD_QUALITY);
}

for (int cpu_used = -16; cpu_used <= -5; ++cpu_used) {
EncodeOssFuzz69906(cpu_used, VPX_DL_REALTIME);
}
}
#endif // CONFIG_VP8_ENCODER

// Set up 2 spatial streams with 2 temporal layers per stream, and generate
Expand Down
2 changes: 2 additions & 0 deletions test/test-data.mk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ LIBVPX_TEST_DATA-$(CONFIG_ENCODERS) += park_joy_90p_8_422.y4m
LIBVPX_TEST_DATA-$(CONFIG_ENCODERS) += park_joy_90p_8_444.y4m
LIBVPX_TEST_DATA-$(CONFIG_ENCODERS) += park_joy_90p_8_440.yuv

LIBVPX_TEST_DATA-$(CONFIG_VP8_ENCODER) += repro-oss-fuzz-69906.y4m

LIBVPX_TEST_DATA-$(CONFIG_VP9_ENCODER) += desktop_credits.y4m
LIBVPX_TEST_DATA-$(CONFIG_VP9_ENCODER) += niklas_1280_720_30.y4m
LIBVPX_TEST_DATA-$(CONFIG_VP9_ENCODER) += noisy_clip_640_360.y4m
Expand Down
1 change: 1 addition & 0 deletions test/test-data.sha1
View file
Original file line number Diff line number Diff line change
Expand Up @@ -871,3 +871,4 @@ d3964f9dad9f60363c81b688324d95b4ec7c8038 *invalid-bug-148271109.ivf.res
ad18ca16f0a249fb3b7c38de0d9b327fed273f96 *hantro_collage_w352h288_nv12.yuv
8a0b2c350539859463d3546a67876c83ff6ff0ac *desktopqvga.320_240.yuv
ad9942a073e245585c93f764ea299382a65939a7 *crowd_run_360p_10_150f.y4m
f9a73e921552598a5804911e9f84fec2318e056a *repro-oss-fuzz-69906.y4m
36 changes: 18 additions & 18 deletions vp8/encoder/mcomp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,8 @@ int vp8_find_best_sub_pixel_step_iteratively(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
offset = (bestmv->as_mv.row) * y_stride + bestmv->as_mv.col;

/* central mv */
bestmv->as_mv.row *= 8;
bestmv->as_mv.col *= 8;
bestmv->as_mv.row = clamp(bestmv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
bestmv->as_mv.col = clamp(bestmv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

/* calculate central point error */
besterr = vfp->vf(y, y_stride, z, b->src_stride, sse1);
Expand Down Expand Up @@ -347,8 +347,8 @@ int vp8_find_best_sub_pixel_step_iteratively(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
tc = bc;
}

bestmv->as_mv.row = br * 2;
bestmv->as_mv.col = bc * 2;
bestmv->as_mv.row = clamp(br * 2, SHRT_MIN, SHRT_MAX);
bestmv->as_mv.col = clamp(bc * 2, SHRT_MIN, SHRT_MAX);

if ((abs(bestmv->as_mv.col - ref_mv->as_mv.col) > (MAX_FULL_PEL_VAL << 3)) ||
(abs(bestmv->as_mv.row - ref_mv->as_mv.row) > (MAX_FULL_PEL_VAL << 3))) {
Expand Down Expand Up @@ -400,8 +400,8 @@ int vp8_find_best_sub_pixel_step(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
#endif

/* central mv */
bestmv->as_mv.row *= 8;
bestmv->as_mv.col *= 8;
bestmv->as_mv.row = clamp(bestmv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
bestmv->as_mv.col = clamp(bestmv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);
startmv = *bestmv;

/* calculate central point error */
Expand Down Expand Up @@ -696,8 +696,8 @@ int vp8_find_best_half_pixel_step(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
#endif

/* central mv */
bestmv->as_mv.row *= 8;
bestmv->as_mv.col *= 8;
bestmv->as_mv.row = clamp(bestmv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
bestmv->as_mv.col = clamp(bestmv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);
startmv = *bestmv;

/* calculate central point error */
Expand Down Expand Up @@ -1123,8 +1123,8 @@ int vp8_diamond_search_sad_c(MACROBLOCK *x, BLOCK *b, BLOCKD *d, int_mv *ref_mv,
}
}

this_mv.as_mv.row = best_mv->as_mv.row * 8;
this_mv.as_mv.col = best_mv->as_mv.col * 8;
this_mv.as_mv.row = clamp(best_mv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
this_mv.as_mv.col = clamp(best_mv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

return fn_ptr->vf(what, what_stride, best_address, in_what_stride, &thissad) +
mv_err_cost(&this_mv, center_mv, mvcost, x->errorperbit);
Expand Down Expand Up @@ -1273,8 +1273,8 @@ int vp8_diamond_search_sadx4(MACROBLOCK *x, BLOCK *b, BLOCKD *d, int_mv *ref_mv,
}
}

this_mv.as_mv.row = best_mv->as_mv.row * 8;
this_mv.as_mv.col = best_mv->as_mv.col * 8;
this_mv.as_mv.row = clamp(best_mv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
this_mv.as_mv.col = clamp(best_mv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

return fn_ptr->vf(what, what_stride, best_address, in_what_stride, &thissad) +
mv_err_cost(&this_mv, center_mv, mvcost, x->errorperbit);
Expand Down Expand Up @@ -1363,8 +1363,8 @@ int vp8_full_search_sad(MACROBLOCK *x, BLOCK *b, BLOCKD *d, int_mv *ref_mv,
}
}

this_mv.as_mv.row = best_mv->as_mv.row * 8;
this_mv.as_mv.col = best_mv->as_mv.col * 8;
this_mv.as_mv.row = clamp(best_mv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
this_mv.as_mv.col = clamp(best_mv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

return fn_ptr->vf(what, what_stride, bestaddress, in_what_stride, &thissad) +
mv_err_cost(&this_mv, center_mv, mvcost, x->errorperbit);
Expand Down Expand Up @@ -1441,8 +1441,8 @@ int vp8_refining_search_sad_c(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
}
}

this_mv.as_mv.row = ref_mv->as_mv.row * 8;
this_mv.as_mv.col = ref_mv->as_mv.col * 8;
this_mv.as_mv.row = clamp(ref_mv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
this_mv.as_mv.col = clamp(ref_mv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

return fn_ptr->vf(what, what_stride, best_address, in_what_stride, &thissad) +
mv_err_cost(&this_mv, center_mv, mvcost, x->errorperbit);
Expand Down Expand Up @@ -1552,8 +1552,8 @@ int vp8_refining_search_sadx4(MACROBLOCK *x, BLOCK *b, BLOCKD *d,
}
}

this_mv.as_mv.row = ref_mv->as_mv.row * 8;
this_mv.as_mv.col = ref_mv->as_mv.col * 8;
this_mv.as_mv.row = clamp(ref_mv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
this_mv.as_mv.col = clamp(ref_mv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);

return fn_ptr->vf(what, what_stride, best_address, in_what_stride, &thissad) +
mv_err_cost(&this_mv, center_mv, mvcost, x->errorperbit);
Expand Down
22 changes: 14 additions & 8 deletions vp8/encoder/pickinter.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ int vp8_skip_fractional_mv_step(MACROBLOCK *mb, BLOCK *b, BLOCKD *d,
(void)mvcost;
(void)distortion;
(void)sse;
bestmv->as_mv.row *= 8;
bestmv->as_mv.col *= 8;
bestmv->as_mv.row = clamp(bestmv->as_mv.row * 8, SHRT_MIN, SHRT_MAX);
bestmv->as_mv.col = clamp(bestmv->as_mv.col * 8, SHRT_MIN, SHRT_MAX);
return 0;
}

Expand Down Expand Up @@ -380,12 +380,18 @@ static void update_mvcount(MACROBLOCK *x, int_mv *best_ref_mv) {
/* Split MV modes currently not supported when RD is nopt enabled,
* therefore, only need to modify MVcount in NEWMV mode. */
if (xd->mode_info_context->mbmi.mode == NEWMV) {
x->MVcount[0][mv_max + ((xd->mode_info_context->mbmi.mv.as_mv.row -
best_ref_mv->as_mv.row) >>
1)]++;
x->MVcount[1][mv_max + ((xd->mode_info_context->mbmi.mv.as_mv.col -
best_ref_mv->as_mv.col) >>
1)]++;
const int row_val =
((xd->mode_info_context->mbmi.mv.as_mv.row - best_ref_mv->as_mv.row) >>
1);
const int row_idx = mv_max + row_val;
const int col_val =
((xd->mode_info_context->mbmi.mv.as_mv.col - best_ref_mv->as_mv.col) >>
1);
const int col_idx = mv_max + col_val;
if (row_idx >= 0 && row_idx < MVvals && col_idx >= 0 && col_idx < MVvals) {
x->MVcount[0][row_idx]++;
x->MVcount[1][col_idx]++;
}
}
}

Expand Down
37 changes: 25 additions & 12 deletions vp8/encoder/rdopt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1564,21 +1564,34 @@ static void rd_update_mvcount(MACROBLOCK *x, int_mv *best_ref_mv) {

for (i = 0; i < x->partition_info->count; ++i) {
if (x->partition_info->bmi[i].mode == NEW4X4) {
x->MVcount[0][mv_max + ((x->partition_info->bmi[i].mv.as_mv.row -
best_ref_mv->as_mv.row) >>
1)]++;
x->MVcount[1][mv_max + ((x->partition_info->bmi[i].mv.as_mv.col -
best_ref_mv->as_mv.col) >>
1)]++;
const int row_val = ((x->partition_info->bmi[i].mv.as_mv.row -
best_ref_mv->as_mv.row) >>
1);
const int row_idx = mv_max + row_val;
const int col_val = ((x->partition_info->bmi[i].mv.as_mv.col -
best_ref_mv->as_mv.col) >>
1);
const int col_idx = mv_max + col_val;
if (row_idx >= 0 && row_idx < MVvals && col_idx >= 0 &&
col_idx < MVvals) {
x->MVcount[0][row_idx]++;
x->MVcount[1][col_idx]++;
}
}
}
} else if (x->e_mbd.mode_info_context->mbmi.mode == NEWMV) {
x->MVcount[0][mv_max + ((x->e_mbd.mode_info_context->mbmi.mv.as_mv.row -
best_ref_mv->as_mv.row) >>
1)]++;
x->MVcount[1][mv_max + ((x->e_mbd.mode_info_context->mbmi.mv.as_mv.col -
best_ref_mv->as_mv.col) >>
1)]++;
const int row_val = ((x->e_mbd.mode_info_context->mbmi.mv.as_mv.row -
best_ref_mv->as_mv.row) >>
1);
const int row_idx = mv_max + row_val;
const int col_val = ((x->e_mbd.mode_info_context->mbmi.mv.as_mv.col -
best_ref_mv->as_mv.col) >>
1);
const int col_idx = mv_max + col_val;
if (row_idx >= 0 && row_idx < MVvals && col_idx >= 0 && col_idx < MVvals) {
x->MVcount[0][row_idx]++;
x->MVcount[1][col_idx]++;
}
}
}

Expand Down

0 comments on commit cdf8da4

Please sign in to comment.