[vlc-devel] [vlc-commits] update: use U32_AT

[Rafaël Carré]

Le 18 nov. 2014 13:21, "Rémi Denis-Courmont" <remi at remlab.net> a écrit :
>
> Le 2014-11-18 13:50, Rafaël Carré a écrit :
>
>>> Indeed the original code is undefined while the new code is defined
from ISO
>>> C point of view (possibly not compatible with OpenPGP though).
>>
>>
>> Our users don't care about ISO C.
>
>
> Some users do. And some users also care about the off-by-one read
overflow.
>
>
>>> >so obviously untested.
>>>
>>> Indeed I do not usually test that code remains undefined, since I rarely
>>> strive for undefined code.
>>
>>
>> You should have read the spec for that code before changing it
>
>
> I doubt that the specification would have helped me determine which of
the buffer length predicate and the buffer read was off-by-one and which
was right. I was fixing the integer overflow, not the off-by-one which I
had not even noticed.

Well the spec clearly mentions 3 encoding schemes: 1 2 and 5 bytes.

I actually noticed the bug solely by looking at the above if (*p > 255), so
I doubt you spent much time at all reading that code.

Please pay attention next time and don't hesitate to ask me rather than
communicate by ways of git push.

> It is obvious that I did not test this beyond compilation. But it is also
obvious that this code is poorly tested and poorly maintained - as is much
of VLC due to lack of resources and automated test cases (I do not need to
remind you that SRTP has unit tests to try to avoid this kind of problems).

Yes tests are nice.
Their absence is not an excuse for regressions though, especially in that
code.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20141118/8d6d6ce8/attachment.html>