[Ffmpeg-devel] Re: [xine-devel] Suspicious code in xine-lib CVS from 2006-04-16 18:43

[Michael Niedermayer]

Hi

non ffmpeg stuff snipped ...

On Tue, Apr 18, 2006 at 08:49:05PM +0200, Christoph Bartoschek wrote:
> Hi,
> 
> here is a report of items brought up by a static code checker. The tool is 
> internal and not intended for the public. If you have questions about some 
> items, please ask me directly.
> 
> ------------------------------------------------------------------
> Misc problems:
> ------------------------------------------------------------------
> 
> - src/libffmpeg/libavcodec/dv.c:888
> 
> pbs[6*5] is out of bounds.
> 
> - src/libffmpeg/libavcodec/dv.c:873
> 
> If j reaches 29, then j+1 is out of bounds of pbs.

NULL is also out of bounds and its use wont crash your program
its dereferencing which does and that doesnt happen here


> 
> - src/libffmpeg/libavcodec/asv1.c:293
> 
> When line 287 is true then ccp becomes 8 and the access is out of
> bounds.

your checker has the same bug as coverity, this isnt possible


> 
> - src/libffmpeg/libavcodec/h261.c:267
> 
> If cbp is 0 as indicated by line 238, then the access is out of bounds.

not possible i think, added a assert() just to be sure


> 
> - src/libffmpeg/libavcodec/h263.c:1624
> 
> Index -1 is invalid.

maybe if you read the c standard very strictly, i dunno, but in practice this
is correct and intended that way, feel free to send a patch if it bothers you


> 
> - src/libffmpeg/libavcodec/h264.c:7016
> 
> Is the ; intended?

probably not, fixed


> 
> - src/libffmpeg/libavcodec/h264.c:6421
> 
> Out of bounds access for i bigger than 3.

svn ffmpeg is not affected


> 
> - src/libffmpeg/libavcodec/h264.c:7060 
>  
>  If aspect_ratio_idc is 14 or 15, you have an out of bounds access.

svn ffmpeg is not affected


> 
> - src/libffmpeg/libavcodec/ffv1.c:380
> 
> sample[2] is out of bounds.

no i dont think so


> 
> - src/libffmpeg/libavcodec/h264.c:4945
> 
> mv_cache[8] is out of bounds.

i doubt that (assuming i found the corresponding line in ffmpeg svn)


> 
> - src/libffmpeg/libavcodec/imgconvert.c:1929
> 
> size is signed.

no clue what the problem is or which line that is in ffmpeg svn


> 
> 
> - src/libffmpeg/libavcodec/mpegaudiodec.c:1507
> 
> If bits is 0 then line 1483 selects the else case and you get an invalid
> shift amount here.

not possible


> 
> - src/libffmpeg/libavcodec/qdm2.c:1473,1476,1478
> 
> Variable i already controls the loop in line 1439.

yep that doesnt look good, ill leave this and the other qdm2.c issues to the 
qdm2.c author/maintainer


> 
> - src/libffmpeg/libavcodec/qdm2.c:541 
> 
> If coding_method[ch][sb][j] - 8 is between 15 and 22 then
> coding_method[ch][sb][j] is between 23 and 30 and you access beyond
> array bounds here, because switchtable has only 23 entries.


> - src/libffmpeg/libavcodec/rangecoder.c:114
> 
> Off by one error, if i is 0.  Then  c->one_state[256-0] is beyond the
> array bounds.

ffmpeg svn is not affected


> 
> - src/libffmpeg/libavcodec/svq1.c:1020
> 
> Out of bound access. 1+count is always bigger than 1 the maximum
> allowed index.

disagree


> 
> - src/libffmpeg/libavcodec/utils.c:881
> 
> && 0 is always false

yes


> 
> - src/libffmpeg/libavcodec/vp3.c:1305
> 
> fragment->next_coeff[coeff_index] is a Coeff and not an int 

this is just in a "debuging printf", ill leave this to the vp3 maintainer


> 
> 
> - src/libffmpeg/libavcodec/ratecontrol.c:909
> 
> Using abs instead of fabs here will loose precision.

fixed


> 
> - src/libffmpeg/libavcodec/ratecontrol.c:844 
> 
> Only avg_quantizer[P_TYPE], avg_quantizer[I_TYPE] and
> avg_quantizer[B_TYPE] are initialized here. The other two are
> uninitialized.

IPB should be all types there are ...


> 
> - src/libffmpeg/libavcodec/truemotion1.c:375
> 
> header.width, header.height, header.yoffset and header.xoffset are not
> initialized here. 
> 
> - src/libffmpeg/libavcodec/truemotion1.c:412
> 
> If header.compression is 17 then this line is an off by one error. Note
> that 17 is not excluded by the earlier check.

ill leave these to the truemotion1 author/maintainer


> 
[...]
> ------------------------------------
> Problems involving the NULL pointer:
> ------------------------------------
> 
> - src/libffmpeg/libavcodec/qdm2.c:1454
> 
> If line 1447 is never executed, then packet is NULL here.
> 
[...]
> -----------------------------------------------------------------
> Cases from switch statements that fall through in some cases but 
> do not have a fall through comment as in most such cases.
> ------------------------------------------------------------------

sorry but wtf is a fall through comment? on which page of the c standard
is that mentioned?

> 
[...]
> - src/libffmpeg/libavcodec/utils.c:231
> - src/libffmpeg/libavcodec/utils.c:226
> - src/libffmpeg/libavcodec/rpza.c:159
> - src/libffmpeg/libavcodec/indeo3.c:1002
> - src/libffmpeg/libavcodec/indeo3.c:999
> 
> -----------------------------------------------------------------
> Lines where boolean expressions are used in non-boolean contexts:
> 
> I suspect that at least the lines marked with !!! are bugs
> -----------------------------------------------------------------
> 
[...]
> - src/libffmpeg/libavutil/rational.c:36

another random piece of correct code ...

[...]

-- 
Michael

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is