svcenc: Integer overflow in irc_ba_get_cur_frm_est_texture_bits #75
Conversation
|
@AshwinNatesan-ittiam the issue being addressed here may be related to oss-fuzz:63044. There is another CL in internal review that is being validated to that issue. Maybe it is better to wait for that fix to land and see if this change is really needed. |
I will wait for the other bug to be fixed. I will also push the changes you asked for after the other bug is fixed, if needed. |
AshwinNatesan-ittiam
force-pushed
the
ossFuzz-63175
branch
from
41c0374 to
7f2880b
Compare
|
I have rebased the branch atop main. The latest commit doesn't fix the overflow in svcenc. I have also included all of the changes requested for in the review. |
encoder/irc_bit_allocation.c
Outdated
| i4_est_texture_bits_for_frm; | ||
| if(I_PIC == e_pic_type) | ||
| { | ||
| WORD64 i4_i_pic_bits = |
There was a problem hiding this comment.
Ashwin,
We would need to investigate a bit more on why i4_i_pic_bits going beyond INT32_MAX.
what were the values of bitrate, framerate that lead to this high value and if something can be checked in svc encoder's configure level that would be ideal.
Also note, this doesn't show up in avc encoder, so maybe svc encoder is indeed missing some checks during initial set up?
There was a problem hiding this comment.
I will gather some data and see how this can be detected during init.
There was a problem hiding this comment.
I looked deeper and it seems like the issue was being caused by incorrect handling of constraints on the order of control commands, specifically set_dimensions and set_frame_rate. These are expected to occur in that order since the first one eventually triggers rc_init, which initialises frame_time struct used by a function eventually invoked by set_frame_rate.
Also, I noticed the CI fuzzer failures with this change. I am looking into them.
AshwinNatesan-ittiam
force-pushed
the
ossFuzz-63175
branch
from
7f2880b to
a97b0cc
Compare
|
Instead of rejecting initializations if they are not in the desired order, would it benefit if all the init commands are captured and applied at once? |
Yes, I agree this is a better approach. But, what happens if one of the control commands, for example the set_dimensions command, fails? In this PR's bug, this would cause the frame_rate used by RC to be set to some garbage value, since the fuzzer doesn't check errors for any of the control commands. Shouldn't we then still be checking for the ordering of at least the two control commands here? |
|
I assume the set dimensions sets resolution of current encode. If this fails, then codec will be in un init state. Any further encode calls should return with error. Any error that is fatal puts component in permanent error state and encode calls shall be rejected. |
Note that there was no init failure in the fuzzer test that resulted in this PR. Init failures will result in fatal errors in svcenc and svc_enc_fuzzer as well. The 'ISVCE_CMD_CTL_SET_DIMENSIONS' was failing because the input dimensions were incorrect. But in 'codec_update_config', calls to rc_init is invoked only when call to 'ISVCE_CMD_CTL_SET_DIMENSIONS' returns successfully. |
'ISVCE_CMD_CTL_SET_DIMENSIONS' is expected to occur before 'ISVCE_CMD_CTL_SET_FRAMERATE'. This is necesssary since 'isvce_rc_init' initialises the frame_time_t struct used by 'ih264e_frame_time_update_src_frame_rate'. This state was not being handled correctly, and consequently, many of the computations in RC were using incorrectly initialised values for frame rate. This was resulting in signed integer overflow of 'i4_est_texture_bits_for_frm'. Bug = ossfuzz:63175 Test: svc_enc_fuzzer
AshwinNatesan-ittiam
force-pushed
the
ossFuzz-63175
branch
from
a97b0cc to
03c4cb7
Compare
RC was incorrectly handling cases where 'I_TO_P_BIT_RATIO' multiplied by 'i4_est_texture_bits_for_frm' (which stores the estimated texture bits for the current frame), exceeded the range of int32_t. This has been fixed by clipping the product of the two quantities above.
Bug = ossfuzz:63175
Test: svc_enc_fuzzer