[FFmpeg-devel] [RFC] ff_huff_build_tree depends on uninitialized data+

[Aurelien Jacobs]

On Fri, 30 Nov 2007 19:47:06 +0100
Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> Hello,
> On Fri, Nov 30, 2007 at 09:14:35PM +0200, Kostya wrote:
> > On Fri, Nov 30, 2007 at 06:57:31PM +0100, Reimar D?ffinger wrote:
> > > Hello,
> > > that function has the following code
> > > >    cur_node = nb_codes;
> > > >    for(i = 0; i < nb_codes*2-1; i += 2){
> > > >        nodes[cur_node].sym = HNODE;
> > > >        nodes[cur_node].count = nodes[i].count + nodes[i+1].count;
> > > >        nodes[cur_node].n0 = i;
> > > >        for(j = cur_node; j > 0; j--){
> > > >            if(nodes[j].count > nodes[j-1].count ||
> > > 
> > > 
> > > Only the first nb_codes of nodes.count must be initialized.
> > > Assume that nb_codes == 1.
> > > Then
> > > > nodes[1].count = nodes[0].count + nodes[1].count;
> > > will be executed, which is undefined.
> > > And a few lines down, nodes[1].count is compared against nodes[0].count.
> > > There are obviously load of ways to fix it, the simples being probably
> > > to do
> > > > nodes[2*nb_codes-1].count = 0;
> > > somewhere before, but I am not sure if that is correct.
> > > Could someone please look at it?
> > > I think this might be what causes the crash in the vp6 codec in issue
> > > 275.
> > 
> > count is uint32_t, so nodes[1].count >= nodes[0].count, I'm not sure about
> 
> Certainly not, if nodes[1].count somehow happens to be 0xffffffff then
> nodes[1].count == nodes[0].count - 1
> 
> > swap but the second node will be ignored anyway, it worked on one 1-bit code.
> 
> Well, if this happens as above, then nodes[0] will be completely
> undefined, if not nodes[1] will be.
> Either way, _the code is wrong_, just try adding
> > nodes[nb_codes*2-1].count = -1;
> at the top of the function and suddenly decoding vp6_crash.avi will
> crash.

Indeed, the code is buggy.

> Thus I intend to apply attached patch somewhen soon.

This patch looks good to me. Let's just commit it.

Aurel