On Thu, Jul 26, 2012 at 8:37 PM, Martin Storsjö <[email protected]> wrote: > On Thu, 26 Jul 2012, Samuel Pitoiset wrote: > >> On Thu, Jul 26, 2012 at 8:31 PM, Samuel Pitoiset >> <[email protected]> wrote: >>> >>> On Thu, Jul 26, 2012 at 8:25 PM, Martin Storsjö <[email protected]> wrote: >>>> >>>> On Thu, 26 Jul 2012, Samuel Pitoiset wrote: >>>> >>>>> On Thu, Jul 26, 2012 at 8:17 PM, Martin Storsjö <[email protected]> >>>>> wrote: >>>>>> >>>>>> >>>>>> On Thu, 26 Jul 2012, Samuel Pitoiset wrote: >>>>>> >>>>>>> On Thu, Jul 26, 2012 at 7:27 PM, Martin Storsjö <[email protected]> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Thu, 26 Jul 2012, Samuel Pitoiset wrote: >>>>>>>> >>>>>>>>> --- >>>>>>>>> libavformat/rtmpproto.c | 6 ++++++ >>>>>>>>> 1 file changed, 6 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c >>>>>>>>> index a2efe38..a32c4a9 100644 >>>>>>>>> --- a/libavformat/rtmpproto.c >>>>>>>>> +++ b/libavformat/rtmpproto.c >>>>>>>>> @@ -915,6 +915,12 @@ static int handle_ping(URLContext *s, >>>>>>>>> RTMPPacket >>>>>>>>> *pkt) >>>>>>>>> >>>>>>>>> t = AV_RB16(pkt->data); >>>>>>>>> if (t == 6) { >>>>>>>>> + if (pkt->data_size < 6) { >>>>>>>>> + av_log(s, AV_LOG_ERROR, "Too short ping packet >>>>>>>>> (%d)\n", >>>>>>>>> + pkt->data_size); >>>>>>>>> + return AVERROR_INVALIDDATA; >>>>>>>>> + } >>>>>>>>> + >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> The commit and warning messages are good this time, however the code >>>>>>>> itself >>>>>>>> is wrong in two different ways. Where did you get the number 6, and >>>>>>>> why >>>>>>>> do >>>>>>>> you do the check here? >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> I played a stream using a FMS and I used Wireshark in order to find >>>>>>> the size of a ping packet. >>>>>>> >>>>>>> I do the check here because that handle function can received other >>>>>>> packets like a swfverification request, for example. And if this >>>>>>> packet is not 6 bytes long a *wrong* error code is returned. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Your argumentation is flawed, and so is your way of figuring out the >>>>>> size >>>>>> limit. >>>>>> >>>>>> Why do we check the size of buffers? >>>>> >>>>> >>>>> >>>>> We check the size of buffers in order to prevent reading outside an >>>>> allocated buffer. >>>> >>>> >>>> >>>> Correct. Now in the handle_ping function, where do we read from the >>>> buffer >>>> we got from the network? >>> >>> >>> Got it! The check should be added before 't = AV_RB16(pkt->data);' >>> otherwise we could read outside of the buffer... >> >> >> Wrong! We have to check if the buffer is equal or more than 2 bytes >> long before extracting the type... >> Do you agree? > > > Exactly. > > There's no weird magic around, and you don't need to use wireshark or any > extra tools for figuring it out. If you read element [x] in an array, you > need to make sure that x < len, before you read it. Always. Nothing more, > nothing less.
Indeed. Thanks for your very good explanations. =) -- Best regards, Samuel Pitoiset. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
