958988 - OpenH264: stack-buffer-overflow crash [@WelsSVCEnc::RcInitTlWeight]

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

codec/encoder/core/src/ratectl.cpp:196

void RcInitTlWeight (sWelsEncCtx* pEncCtx) {
  SWelsSvcRc* pWelsSvcRc = &pEncCtx->pWelsSvcRc[pEncCtx->uiDependencyId];
  SRCTemporal* pTOverRc = pWelsSvcRc->pTemporalOverRc;
  SDLayerParam* pDLayerParam =  &pEncCtx->pSvcParam->sDependencyLayers[pEncCtx->uiDependencyId];
  const int32_t kiDecompositionStages = pDLayerParam->iDecompositionStages;
  const int32_t kiHighestTid = pDLayerParam->iHighestTemporalId;

  [...]
  
  n = 0;
  while (n <= kiHighestTid) {
*   pTOverRc[n].dTlayerWeight   = WeightArray[kiDecompositionStages][n];
    ++ n;
  }


To reproduce do: 

  ./h264enc welsenc_fuzz.cfg

Happened accidentally while auto generating a new configuration.

Tested with https://github.com/cisco/openh264/commit/4a8a9aabc1

Comment 1

[Christoph Diehl [:posidron]]

Attachment: welsenc_fuzz.cfg

Comment 2

[Christoph Diehl [:posidron]]

Attachment: layer1_fuzz.cfg

Comment 3

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 4

[Eric Rescorla (:ekr)]

Taking a quick look at this, it appears that this may be a bogus configuration. Which isn't to say it should crash :)

Comment 5

[Christoph Diehl [:posidron]]

I am yet unclear about what all these flags exactly do but I believe it is related to the GOPSize entry.
Using 16 here crashes, whereby 4 makes no trouble.

Comment 6

[karina]

I checked code, there is no overflows for "WeightArray" variable.

is it a false positive?  I found the blew hint when running code.

" HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)"

Comment 7

[Christoph Diehl [:posidron]]

(In reply to karina from comment #6)
> is it a false positive?  I found the blew hint when running code.

Seems possible, "kiHighestTid" and "kiDecompositionStages" are both 4.

Can we add some code to prevent ASan from complaining about it?

Comment 8

[Christoph Diehl [:posidron]]

Wait. It's late here. Shouldn't that be:

while (n < kiHighestTid) {
  ...
}

Comment 9

[karina]

got it. I found the root cause for this.

there is some potential issues to support GOPsize = 16. We will discuss whether we close GOPsize = 16.  
will update it later. thanks for you help.

Comment 10

[karina]

Hi Christoph:
   fix it. Could you verify it on cisco/master branch? thanks!

Comment 11

[Christoph Diehl [:posidron]]

Fixed.

Tested with https://github.com/cisco/openh264/commit/de935a523