[vlc-devel] [vlc-commits] auhal: add missing bounds checking (close #10110)

[David Fuhrmann]

Hi,

Am 27.12.2013 um 10:55 schrieb Felix Paul Kühne <fkuehne at videolan.org>:

> 
> Seeing the crash logs (~9,500 of them since the 2.1.2 release), memcpy doesn't really seem to like copying a zero buffer.

According to my research, memcpy with 0 is fine. Thus, the pointers must be wrong for this crash. buffer should be valid if availableBytes != 0 (previously checked), and targetBuffer should be valid if mDataByteSize is > 0. This should always be true, but if you want it might be better to directly check this variable in the beginning.

While it may be ok for a quick fix, generally I do not like code which tries to hide potential bugs, instead of properly fixing them. (This especially also applies to your other patch, where the extra check for the buffer size makes even less sense, for me).

> 
>> So I assume this bug might be also just fixed with improved locking in your previous patch.
> 
> This should be the case, but for sakes of stability and since I can't be sure that there is no corner-case racing condition, I'd have an extra level of stability assurance here.

Sorry, but this makes no sense. If there is a potential race condition, your additional check brings nothing. The race condition can still happen between the if() and the memcpy.

> 
>> And if this patch is actually needed: what about the SPDIF callback?
> 
> SPDIF is used way less than the "analog" output so logs may be rare. I'll scan the crash reports and adapt the alternative code path if needed.

As I see it, the situation for the SPIDF callback should be the same (same flush implementation, same Play, same reading).

With best regards,
David