[vlc-devel] [PATCH] src: input/stream: fix buffer underflow by avoiding inconsistent state in error case

[David Fuhrmann]

Am 10.06.2014 um 23:51 schrieb David Fuhrmann <david.fuhrmann at gmail.com>:

> 
> Am 10.06.2014 um 22:35 schrieb Rémi Denis-Courmont <remi at remlab.net>:
> 
>> Le mardi 10 juin 2014, 22:30:48 David Fuhrmann a écrit :
>>> Actually, I’m fine to only committing the first chunk to fix the underflow
>>> for a start, as its sufficient for the bug.
>> 
>> The sign overflow fix is the second chunk.
> 
> I fail to see how the first chunk introduces a „refill hack“. Thus I thought you meant the second chunk. If you want to call it a hack, the hack is already there (stream.c lines 1254 up to 1269) and the underflow within the refill function was ignored so far. Additionally, the exact error case I have was already sort of taken into account, with the i_pos >= tk->i_end condition.
> As the error path probably was only rarely executed, the problem in this path was undetected up to now. (Note that I also can only reproduce the problem in very strict circumstances.) Now, this is a patch for a proposal for an intermediate solution. If you (or someone else) want to rewrite / fix the whole „hack“ as it is currently in the code, you are welcome to do so. But until then, we should use the proposed patch.
> 
>> The sign overflow fix is the second chunk.
> 
> And I doubt that only applying the second chunk to all occurences (they are more) is a proper solution. Actually, this would be the real workaround, as the root of the issue is at (or above the first chunk), as it currently stands. And this change might break even more functionality.


Hello,

It would be great if more devs could look at the issue and give comments.

Generally, we have the case that there is an error path which can leave wrongly initialized data (i_offset). If I understand the code correctly, the offset should always stay lower or equal than buffer->end - buffer->start. According to a couple of lines before the patch, its obvious that in the error path i_offset can be invalid.

The proposed fix now just corrects i_offset (in the first chunk). I think its common practice that you do not leave state invalid in error paths, if this state is accessed afterwards. For instance, you would also always set dangling pointers to NULL.
Now, having the existing code, and seeing that the existing condition in the if directly points to the fact that i_offset will probably be invalid, I propose that we should correct it here, for a start.

But I agree with Remi that this is may be not the best solution for the problem. Particularly, i_offset could be wrong even if AStreamRefillStream returns 0. We can also catch this case, but the question is if the function should also return VLC_EGENERIC then.

As I’m not knowing this part of the code base good enough, can anyone else give some light here?

Particulary, I’m asking myself the following questions:

1) Is it absolutely necessary, that i_offset always stays within the range [0, i_end - i_start] (for the currently selected buffer)? Is it allowed that this variable stays outside temporarily?

2) Seeing these conditions for refilling the buffer (stream.c:1259):
    if( tk->i_end < tk->i_start + p_sys->stream.i_offset + p_sys->stream.i_read_size )

Now, this condition can be true if  tk->i_start + p_sys->stream.i_offset <  tk->i_end, and i_read_size is big enough. Or, stream.i_offset is already too large, and its irrelevant what value i_read_size has. Which of the both interpretations was intended?

With best regards,
David