[XviD-devel] Possible bug in bitstream/mbcoding.c

[Martin Thierer]

On Monday 30 September 2002 16:29, skal wrote:
> On Sat, 2002-09-28 at 22:25, Martin Thierer wrote:
> > Please note that this code hasn't changed since snapshot 20020728, but
> > as little changes in the code also make the problem go away for me,
> > that hasn't much to say. It's probably just a matter of a few bytes if
> > an illegal page is hit or not.
>
> 	Maybe running Valgrind on your faulty binary will help, here.
> 	(http://developer.kde.org/~sewardj). Sounds like a nasty
> 	memory corruption...

I did that. It reports an illegal read of size byte. Unfortunatly valgrind 
doesn't report the exact location, as libxvidcore is compiled without 
debug info. (When I do, it doesn't crash anymore :-) But as I said, I'm 
pretty sure that it crashes at the access of "max_level_ptr[run]".

I took a closer look at the whole init_vls_tables function now, and while I 
still don't understand what it is supposed to do I suspect that it's 
buggy. Please forgive me, if I'm wrong :-)

Things that seem odd to me:

1. The arrays "intra_table" and "inter_table" are both of size 524032. But: 
If I see it correct, in every iteration of the inner loop a value is 
written by vlc[intra]++. That would be 2*(2048-(-2047))*64=524160 elements 
for each table, meaning the arrays would be too small.

2. From the inner loop:

  if ((abs(level) <= max_level_ptr[run]) && 
      (run <= (uint32_t)  max_run_ptr[abs(level)])) {
    vlc[intra]->code = 0;
    vlc[intra]->len = 0;
    goto loop_end;
  } else {
    if (level > 0)	// correct level
      level -= max_level_ptr[run];
    else
      level += max_level_ptr[run];

Is this really correct? It would make sense to me if the first condition in 
the above if-clause failed, but not if only the second one fails, as 
"level" then changes its sign.

3. Also from the inner loop, a little further down:

    run -= max_run_ptr[abs(level)] + 1;	// and change run

Here "run" becomes negative when run <= max_run_ptr[abs(level)]. This 
happens if both "abs(level)" and "abs(abs(level)-max_level_ptr[run])" are 
> max_level_ptr[run] (as otherwise one of the earlier if-conditions would 
have been true). But as "max_level" is defined as "static char 
max_level[4][64]" it's illegal to use a negativ subscript as it is done 
then:

    if ((abs(level) <= max_level_ptr[run]) &&
        (run <= (uint32_t) max_run_ptr[abs(level)])) {

This is where it crashes for me.

As I said, I don't understand what the code is supposed to do, so I 
couldn't really suggest a solution.

Bye,

Martin