Fix issue in SVC parameter configuration Always clamp the spatial/temporal_layer_id in set_svc_params. Current code was only clamping if the number of layers were detected to change. This fixes heap-buffer-overflow when the number of spatial layers are set with AOME_SET_NUMBER_SPATIAL_LAYERS before AV1E_SET_SVC_PARAMS. Additional fixes; -validate layer_id against both cpi->svc and ppi in AV1E_SET_SVC_LAYER_ID -add a clamp on spatial_layer_id in AOME_SET_NUMBER_SPATIAL_LAYERS -add condition on use_svc to return invalid_param in AOME_SET_NUMBER_SPATIAL_LAYERS, to avoid using this control when AV1E_SET_SVC_PARAMS is used. Add unittest. Bug: 538651949 Change-Id: I1ff6d0968b6d6717bf49539f48f10fb4fb2a608a
diff --git a/av1/av1_cx_iface.c b/av1/av1_cx_iface.c index b0b2762..e1e8ef1 100644 --- a/av1/av1_cx_iface.c +++ b/av1/av1_cx_iface.c
@@ -4081,10 +4081,16 @@ // it is false (the default) the actual limit is 3 for both spatial and // temporal layers. Given the order of these calls are unpredictable the // final check is deferred until encoder_encode() (av1_set_svc_fixed_mode()). + // The condition to return invalid_param when use_svc is set + // (set in set_svc_params) is to avoid calling this control when + // set_svc_params is already called (which sets the number of spatial and + // temporal layers). if (number_spatial_layers <= 0 || - number_spatial_layers > MAX_NUM_SPATIAL_LAYERS) + number_spatial_layers > MAX_NUM_SPATIAL_LAYERS || ctx->ppi->use_svc) return AOM_CODEC_INVALID_PARAM; ctx->ppi->number_spatial_layers = number_spatial_layers; + ctx->ppi->cpi->common.spatial_layer_id = clamp( + ctx->ppi->cpi->common.spatial_layer_id, 0, number_spatial_layers - 1); // update_encoder_cfg() is somewhat costly and this control may be called // multiple times, so update_encoder_cfg() is only called to ensure frame and // superblock sizes are updated before they're fixed by the first encode @@ -4100,7 +4106,10 @@ aom_svc_layer_id_t *const data = va_arg(args, aom_svc_layer_id_t *); if (data->spatial_layer_id < 0 || data->temporal_layer_id < 0 || data->spatial_layer_id >= (int)ctx->ppi->number_spatial_layers || - data->temporal_layer_id >= (int)ctx->ppi->number_temporal_layers) { + data->temporal_layer_id >= (int)ctx->ppi->number_temporal_layers || + data->spatial_layer_id >= (int)ctx->ppi->cpi->svc.number_spatial_layers || + data->temporal_layer_id >= + (int)ctx->ppi->cpi->svc.number_temporal_layers) { return AOM_CODEC_INVALID_PARAM; } ctx->ppi->cpi->common.spatial_layer_id = data->spatial_layer_id; @@ -4152,21 +4161,17 @@ ctx->next_frame_flags |= AOM_EFLAG_FORCE_KF; av1_set_svc_seq_params(ppi); av1_free_svc_cyclic_refresh(cpi); - // Check for valid values for the spatial/temporal_layer_id here, since - // there has been a dynamic change in the number_spatial/temporal_layers, - // and if the ctrl_set_layer_id is not used after this call, the - // previous (last_encoded) values of spatial/temporal_layer_id will be used, - // which may be invalid. - cpi->svc.spatial_layer_id = - clamp(cpi->svc.spatial_layer_id, 0, cpi->svc.number_spatial_layers - 1); - cpi->svc.temporal_layer_id = clamp(cpi->svc.temporal_layer_id, 0, - cpi->svc.number_temporal_layers - 1); - cpi->common.spatial_layer_id = clamp(cpi->common.spatial_layer_id, 0, - cpi->svc.number_spatial_layers - 1); - cpi->common.temporal_layer_id = clamp(cpi->common.temporal_layer_id, 0, - cpi->svc.number_temporal_layers - 1); } + // Check for valid values for the spatial/temporal_layer_id here. + // If ctrl_set_layer_id is not used after this call, the currently configured + // values of spatial/temporal_layer_id will be used, which may be out of + // bounds. + cpi->common.spatial_layer_id = cpi->svc.spatial_layer_id = + clamp(cpi->svc.spatial_layer_id, 0, cpi->svc.number_spatial_layers - 1); + cpi->common.temporal_layer_id = cpi->svc.temporal_layer_id = + clamp(cpi->svc.temporal_layer_id, 0, cpi->svc.number_temporal_layers - 1); + if (ppi->number_spatial_layers > 1 || ppi->number_temporal_layers > 1) { unsigned int sl, tl; // Disable svc for lag_in_frames > 0.
diff --git a/test/encode_api_test.cc b/test/encode_api_test.cc index f406bac..e672822 100644 --- a/test/encode_api_test.cc +++ b/test/encode_api_test.cc
@@ -254,6 +254,60 @@ EXPECT_EQ(aom_codec_destroy(&enc), AOM_CODEC_OK); } +// Bug: 538651949. +TEST(EncodeAPI, SvcFixBypass) { + aom_codec_ctx_t enc; + aom_codec_enc_cfg_t cfg; + aom_codec_iface_t *iface = aom_codec_av1_cx(); + aom_svc_layer_id_t layer_id = { 3, 0 }; + aom_svc_params_t svc_params = {}; + + EXPECT_EQ(aom_codec_enc_config_default(iface, &cfg, kUsage), AOM_CODEC_OK); + cfg.g_w = 320; + cfg.g_h = 240; + cfg.g_threads = 1; + cfg.g_lag_in_frames = 0; + + ASSERT_EQ(aom_codec_enc_init(&enc, iface, &cfg, 0), AOM_CODEC_OK); + + // Set number of spatial layers to 4 via control SET_NUMBER_SPATIAL_LAYERS. + EXPECT_EQ(aom_codec_control(&enc, AOME_SET_NUMBER_SPATIAL_LAYERS, 4), + AOM_CODEC_OK); + + // Set the active spatial layer id to 3. This is expected to fail since + // cpi->svc.number_spatial_layers is still 1 (default). + EXPECT_EQ(aom_codec_control(&enc, AV1E_SET_SVC_LAYER_ID, &layer_id), + AOM_CODEC_INVALID_PARAM); + + // Set SVC params to 2x2 layer config, via AV1E_SET_SVC_PARAMS. This sets + // all svc layer parameters, allocates for the layer context, and sets use_svc + // = 1. + svc_params.number_spatial_layers = 2; + svc_params.number_temporal_layers = 2; + for (int i = 0; i < AOM_MAX_LAYERS; i++) { + svc_params.max_quantizers[i] = 52; + svc_params.min_quantizers[i] = 10; + svc_params.layer_target_bitrate[i] = 100; + } + svc_params.scaling_factor_num[0] = 1; + svc_params.scaling_factor_den[0] = 2; + svc_params.scaling_factor_num[1] = 1; + svc_params.scaling_factor_den[1] = 1; + svc_params.framerate_factor[0] = 2; + svc_params.framerate_factor[1] = 1; + + EXPECT_EQ(aom_codec_control(&enc, AV1E_SET_SVC_PARAMS, &svc_params), + AOM_CODEC_OK); + + // Now try setting number of spatial layers to 3 using the control + // SET_NUMBER_SPATIAL_LAYERS. This is expected to fail because SVC was + // configured via AV1E_SET_SVC_PARAMS, so use_svc = 1. + EXPECT_EQ(aom_codec_control(&enc, AOME_SET_NUMBER_SPATIAL_LAYERS, 3), + AOM_CODEC_INVALID_PARAM); + + EXPECT_EQ(aom_codec_destroy(&enc), AOM_CODEC_OK); +} + TEST(EncodeAPI, InvalidControlId) { aom_codec_iface_t *iface = aom_codec_av1_cx(); aom_codec_ctx_t enc;