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

[Reimar Döffinger]

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.
Thus I intend to apply attached patch somewhen soon.

Greetings,
Reimar D?ffinger
-------------- next part --------------
Index: libavcodec/huffman.c
===================================================================
--- libavcodec/huffman.c	(revision 11113)
+++ libavcodec/huffman.c	(working copy)
@@ -82,6 +82,7 @@
     }
     qsort(nodes, nb_codes, sizeof(Node), cmp);
     cur_node = nb_codes;
+    nodes[nb_codes*2-1].count = 0;
     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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071130/0b14965d/attachment.pgp>