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

[Roman Shaposhnik]

On Mon, 2006-05-29 at 19:16 +0200, Michael Niedermayer wrote:
> Hi
> 
> non ffmpeg stuff snipped ...
  
<ffmpeg unrelated question>
Whats up with tool they used ? If its truly "internal and not intended
for the public" why did they bother with ffmpeg/MPlayer in the first 
place ?
</ffmpeg unrelated question>

Thanks,
Roman. 

> 
> 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 ...
> 
> [...]
>