[x264-devel] [PATCH] Fix index out-of-bound bugs

[BugMaster]

On Tue, 31 Mar 2020 17:12:53 -0700, Vitaly Buka wrote:
> From: Vitaly Buka <vitalybuka at gmail.com>

> 1. i_mb_partition does not match loop at encoder/encoder.c:4025
> It looks like it should have the same size as i_mb_count[3].

> 2. lowres_mvs[1][p1-b-1] is fix of "undefined behaviour"
> In some cases index is negative. There is no memory when it happens.
> However, according to the C standard even calculation of such pointer
> is undefined behaviour.
> https://port70.net/~nsz/c/c11/n1570.html#6.5.6p8

> Both were detected with clang -fsanitize=bounds and the fix will help
> to use this code in projects where -fsanitize=bounds is used.
> ---
>  common/common.h     | 2 +-
>  encoder/slicetype.c | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)

> diff --git a/common/common.h b/common/common.h
> index a167df63..06f9144c 100644
> --- a/common/common.h
> +++ b/common/common.h
> @@ -697,7 +697,7 @@ struct x264_t
>          double  f_frame_duration[3];
>          /* */
>          int64_t i_mb_count[3][19];
> -        int64_t i_mb_partition[2][17];
> +        int64_t i_mb_partition[3][17];
>          int64_t i_mb_count_8x8dct[2];
>          int64_t i_mb_count_ref[2][2][X264_REF_MAX*2];
>          int64_t i_mb_cbp[6];
> diff --git a/encoder/slicetype.c b/encoder/slicetype.c
> index a520f586..487727ee 100644
> --- a/encoder/slicetype.c
> +++ b/encoder/slicetype.c
> @@ -519,8 +519,6 @@ static void slicetype_mb_cost( x264_t *h, x264_mb_analysis_t *a,
>      const int i_stride = fenc->i_stride_lowres;
>      const int i_pel_offset = 8 * (i_mb_x + i_mb_y * i_stride);
>      const int i_bipred_weight = h->param.analyse.b_weighted_bipred
> ? 64 - (dist_scale_factor>>2) : 32;
> -    int16_t (*fenc_mvs[2])[2] = {
> &fenc->lowres_mvs[0][b-p0-1][i_mb_xy],
> &fenc->lowres_mvs[1][p1-b-1][i_mb_xy] };
> -    int (*fenc_costs[2]) = {
> &fenc->lowres_mv_costs[0][b-p0-1][i_mb_xy],
> &fenc->lowres_mv_costs[1][p1-b-1][i_mb_xy] };
>      int b_frame_score_mb = (i_mb_x > 0 && i_mb_x < h->mb.i_mb_width - 1 &&
>                              i_mb_y > 0 && i_mb_y < h->mb.i_mb_height - 1) ||
>                              h->mb.i_mb_width <= 2 || h->mb.i_mb_height <= 2;
> @@ -539,6 +537,9 @@ static void slicetype_mb_cost( x264_t *h, x264_mb_analysis_t *a,
>      if( p0 == p1 )
>          goto lowres_intra_mb;
>  
> +    int16_t (*fenc_mvs[2])[2] = {
> &fenc->lowres_mvs[0][b-p0-1][i_mb_xy], p1 == b ? NULL :
> &fenc->lowres_mvs[1][p1-b-1][i_mb_xy] };
> +    int (*fenc_costs[2]) = {
> &fenc->lowres_mv_costs[0][b-p0-1][i_mb_xy], p1 == b ? NULL :
> &fenc->lowres_mv_costs[1][p1-b-1][i_mb_xy] };
> +
>      int mv_range = 2 * h->param.analyse.i_mv_range;
>      // no need for h->mb.mv_min[]
>      h->mb.mv_min_spel[0] = X264_MAX( 4*(-8*h->mb.i_mb_x - 12), -mv_range );
> @@ -1045,7 +1046,7 @@ static void macroblock_tree_propagate( x264_t
> *h, x264_frame_t **frames, float a
>      uint16_t *ref_costs[2] =
> {frames[p0]->i_propagate_cost,frames[p1]->i_propagate_cost};
>      int dist_scale_factor = ( ((b-p0) << 8) + ((p1-p0) >> 1) ) / (p1-p0);
>      int i_bipred_weight = h->param.analyse.b_weighted_bipred ? 64 - (dist_scale_factor>>2) : 32;
> -    int16_t (*mvs[2])[2] = { frames[b]->lowres_mvs[0][b-p0-1],
> frames[b]->lowres_mvs[1][p1-b-1] };
> +    int16_t (*mvs[2])[2] = { frames[b]->lowres_mvs[0][b-p0-1], b
> == p1 ? NULL : frames[b]->lowres_mvs[1][p1-b-1] };
>      int bipred_weights[2] = {i_bipred_weight, 64 - i_bipred_weight};
>      int16_t *buf = h->scratch_buffer;
>      uint16_t *propagate_cost = frames[b]->i_propagate_cost;

Hi.

Thanks for the report but that is already fixed in my fork:
https://code.videolan.org/BugMaster/x264/-/commit/418788e79dbd10c6f37321bca1c4c4320e9a0ffa
Also some other undefined behaviour reported by clang also fixed in
that branch: https://code.videolan.org/BugMaster/x264/-/commits/devel
That will be pushed to official repo when I finish one more patch and
fix commit messages and make code review of all of them.