<p dir="ltr">Le 18 nov. 2014 13:21, "Rémi Denis-Courmont" <<a href="mailto:remi@remlab.net">remi@remlab.net</a>> a écrit :<br>
><br>
> Le 2014-11-18 13:50, Rafaël Carré a écrit :<br>
><br>
>>> Indeed the original code is undefined while the new code is defined from ISO<br>
>>> C point of view (possibly not compatible with OpenPGP though).<br>
>><br>
>><br>
>> Our users don't care about ISO C.<br>
><br>
><br>
> Some users do. And some users also care about the off-by-one read overflow.<br>
><br>
><br>
>>> >so obviously untested.<br>
>>><br>
>>> Indeed I do not usually test that code remains undefined, since I rarely<br>
>>> strive for undefined code.<br>
>><br>
>><br>
>> You should have read the spec for that code before changing it<br>
><br>
><br>
> 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.</p>
<p dir="ltr">Well the spec clearly mentions 3 encoding schemes: 1 2 and 5 bytes.</p>
<p dir="ltr">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.</p>
<p dir="ltr">Please pay attention next time and don't hesitate to ask me rather than communicate by ways of git push.</p>
<p dir="ltr">> 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).</p>
<p dir="ltr">Yes tests are nice.<br>
Their absence is not an excuse for regressions though, especially in that code.</p>