Message ID | 20101215171139.b6c1f03a.ospite@studenti.unina.it (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <mchehab@gaivota> Envelope-to: mchehab@gaivota Delivery-date: Wed, 15 Dec 2010 14:13:10 -0200 Received: from mchehab by gaivota with local (Exim 4.72) (envelope-from <mchehab@gaivota>) id 1PStyX-0005fb-Rz for mchehab@gaivota; Wed, 15 Dec 2010 14:13:10 -0200 Received: from casper.infradead.org [85.118.1.10] by gaivota with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Wed, 15 Dec 2010 14:13:09 -0200 (BRST) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PStxR-0003ce-Q4; Wed, 15 Dec 2010 16:12:02 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752871Ab0LOQL7 (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Wed, 15 Dec 2010 11:11:59 -0500 Received: from smtp209.alice.it ([82.57.200.105]:47287 "EHLO smtp209.alice.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751778Ab0LOQL6 (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 15 Dec 2010 11:11:58 -0500 Received: from jcn (82.61.82.13) by smtp209.alice.it (8.5.124.08) (authenticated as fospite@alice.it) id 4C1A27590CBB9451 for linux-media@vger.kernel.org; Wed, 15 Dec 2010 17:11:57 +0100 Date: Wed, 15 Dec 2010 17:11:39 +0100 From: Antonio Ospite <ospite@studenti.unina.it> To: linux-media@vger.kernel.org Subject: Question about libv4lconvert. Message-Id: <20101215171139.b6c1f03a.ospite@studenti.unina.it> X-Mailer: Sylpheed 3.0.2 (GTK+ 2.20.1; x86_64-pc-linux-gnu) X-Face: z*RaLf`X<@C75u6Ig9}{oW$H; 1_\2t5)({*|jhM<pyWR#k60!#=#>/Vb; ]yA5<GWI5`6u&+ ; 6b'@y|8w"wB; 4/e!7wYYrcqdJFY,~%Gk_4]cq$Ei/7<j&N3ah(m`ku?pX.&+~:_/wC~dwn^)MizBG !pE^+iDQQ1yC6^,)YDKkxDd!T>\I~93>J<_`<4)A{':UrE Mime-Version: 1.0 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg="PGP-SHA1"; boundary="Signature=_Wed__15_Dec_2010_17_11_39_+0100_6q1XD/+JuO6Eul2W" Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org Sender: Mauro Carvalho Chehab <mchehab@gaivota> |
Commit Message
Antonio Ospite
Dec. 15, 2010, 4:11 p.m. UTC
Hi, I am taking a look at libv4lconvert, and I have a question about the logic in v4lconvert_convert_pixfmt(), in some conversion switches there is code like this: case V4L2_PIX_FMT_GREY: switch (dest_pix_fmt) { case V4L2_PIX_FMT_RGB24: case V4L2_PIX_FMT_BGR24: v4lconvert_grey_to_rgb24(src, dest, width, height); break; case V4L2_PIX_FMT_YUV420: case V4L2_PIX_FMT_YVU420: v4lconvert_grey_to_yuv420(src, dest, fmt); break; } if (src_size < (width * height)) { V4LCONVERT_ERR("short grey data frame\n"); errno = EPIPE; result = -1; } break; However the conversion routines which are going to be called seem to assume that the buffers, in particular the source buffer, are of the correct full frame size when looping over them. My question is: shouldn't the size check now at the end of the case block be at the _beginning_ of it instead, so to detect a short frame before conversion and avoid a possible out of bound access inside the conversion routine? Some patches to show what I am saying: Regards, Antonio
Comments
Hi, On 12/15/2010 05:11 PM, Antonio Ospite wrote: > Hi, > > I am taking a look at libv4lconvert, and I have a question about the > logic in v4lconvert_convert_pixfmt(), in some conversion switches there > is code like this: > > case V4L2_PIX_FMT_GREY: > switch (dest_pix_fmt) { > case V4L2_PIX_FMT_RGB24: > case V4L2_PIX_FMT_BGR24: > v4lconvert_grey_to_rgb24(src, dest, width, height); > break; > case V4L2_PIX_FMT_YUV420: > case V4L2_PIX_FMT_YVU420: > v4lconvert_grey_to_yuv420(src, dest, fmt); > break; > } > if (src_size< (width * height)) { > V4LCONVERT_ERR("short grey data frame\n"); > errno = EPIPE; > result = -1; > } > break; > > However the conversion routines which are going to be called seem to > assume that the buffers, in particular the source buffer, are of the > correct full frame size when looping over them. > Correct, because they trust that the kernel drivers have allocated large enough buffers to hold a valid frame which is a safe assumption. > My question is: shouldn't the size check now at the end of the case > block be at the _beginning_ of it instead, so to detect a short frame > before conversion and avoid a possible out of bound access inside the > conversion routine? This is done this way deliberately, this has to do with how the EPIPE errno variable is used in a special way. An error return from v4lconvert_convert with an errno of EPIPE means I managed to get some data for you but not an entire frame. The upper layers of libv4l will respond to this by retrying (getting another frame), but only a limited number of times. Once the retries are exceeded they will simply pass along whatever they did manage to get. The reason for this is that there can be bus errors or vsync issues (*), which lead to a short frame, which are intermittent errors. So detecting them and getting another frame is a good thing to do because usually the next frame will be fine. However sometimes there are cases where every single frame is a short frame, for example the zc3xx driver used to deliver jpeg's with only 224 lines of data when running at 320x240 with some sensors. Now one can argue that this is a driver issue, and it is but it still happens in this case it is much better to pass along the 224 lines which we did get, then to make this a fatal error. Note that due to the retries the user will get a much lower framerate, which together with the missing lines at the bottom + printing of error messages will hopefully be enough for the user to report a bug to us, despite him/her getting some picture. I hope this explains. Regards, Hans *) While starting the stream it may take several frames for vsync to properly lock in some cases. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 15 Dec 2010 21:10:52 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > Hi Hans, thanks for the quick reply. > On 12/15/2010 05:11 PM, Antonio Ospite wrote: > > Hi, > > > > I am taking a look at libv4lconvert, and I have a question about the > > logic in v4lconvert_convert_pixfmt(), in some conversion switches there > > is code like this: > > > > case V4L2_PIX_FMT_GREY: > > switch (dest_pix_fmt) { > > case V4L2_PIX_FMT_RGB24: > > case V4L2_PIX_FMT_BGR24: > > v4lconvert_grey_to_rgb24(src, dest, width, height); > > break; > > case V4L2_PIX_FMT_YUV420: > > case V4L2_PIX_FMT_YVU420: > > v4lconvert_grey_to_yuv420(src, dest, fmt); > > break; > > } > > if (src_size< (width * height)) { > > V4LCONVERT_ERR("short grey data frame\n"); > > errno = EPIPE; > > result = -1; > > } > > break; > > > > However the conversion routines which are going to be called seem to > > assume that the buffers, in particular the source buffer, are of the > > correct full frame size when looping over them. > > > > Correct, because they trust that the kernel drivers have allocated large > enough buffers to hold a valid frame which is a safe assumption. > Maybe this was the piece I was missing: even a partial (useful) frame comes in a full size buffer, right? If so then the current logic is sane indeed; and if the current assumption in conversion routines is contradicted then it must be due to a defect related to the kernel driver. > > My question is: shouldn't the size check now at the end of the case > > block be at the _beginning_ of it instead, so to detect a short frame > > before conversion and avoid a possible out of bound access inside the > > conversion routine? > > This is done this way deliberately, this has to do with how the EPIPE > errno variable is used in a special way. > > An error return from v4lconvert_convert with an errno of EPIPE means > I managed to get some data for you but not an entire frame. The upper > layers of libv4l will respond to this by retrying (getting another frame), > but only a limited number of times. Once the retries are exceeded they > will simply pass along whatever they did manage to get. > Ah, that's why you do the conversion even on partial frames, I think I see now. > The reason for this is that there can be bus errors or vsync issues (*), > which lead to a short frame, which are intermittent errors. So detecting > them and getting another frame is a good thing to do because usually the > next frame will be fine. However sometimes there are cases where every > single frame is a short frame, for example the zc3xx driver used to > deliver jpeg's with only 224 lines of data when running at 320x240 with > some sensors. Now one can argue that this is a driver issue, and it is > but it still happens in this case it is much better to pass along > the 224 lines which we did get, then to make this a fatal error. > > Note that due to the retries the user will get a much lower framerate, > which together with the missing lines at the bottom + printing > of error messages will hopefully be enough for the user to report > a bug to us, despite him/her getting some picture. > > I hope this explains. > It does, thanks a lot. > Regards, > > Hans > > > *) While starting the stream it may take several frames for vsync to > properly lock in some cases. > Regards, Antonio.
Hi, On 12/16/2010 12:49 AM, Antonio Ospite wrote: > On Wed, 15 Dec 2010 21:10:52 +0100 > Hans de Goede<hdegoede@redhat.com> wrote: > >> Hi, >> > > Hi Hans, thanks for the quick reply. > >> On 12/15/2010 05:11 PM, Antonio Ospite wrote: >>> Hi, >>> >>> I am taking a look at libv4lconvert, and I have a question about the >>> logic in v4lconvert_convert_pixfmt(), in some conversion switches there >>> is code like this: >>> >>> case V4L2_PIX_FMT_GREY: >>> switch (dest_pix_fmt) { >>> case V4L2_PIX_FMT_RGB24: >>> case V4L2_PIX_FMT_BGR24: >>> v4lconvert_grey_to_rgb24(src, dest, width, height); >>> break; >>> case V4L2_PIX_FMT_YUV420: >>> case V4L2_PIX_FMT_YVU420: >>> v4lconvert_grey_to_yuv420(src, dest, fmt); >>> break; >>> } >>> if (src_size< (width * height)) { >>> V4LCONVERT_ERR("short grey data frame\n"); >>> errno = EPIPE; >>> result = -1; >>> } >>> break; >>> >>> However the conversion routines which are going to be called seem to >>> assume that the buffers, in particular the source buffer, are of the >>> correct full frame size when looping over them. >>> >> >> Correct, because they trust that the kernel drivers have allocated large >> enough buffers to hold a valid frame which is a safe assumption. >> > > Maybe this was the piece I was missing: even a partial (useful) frame > comes in a full size buffer, right? Right. > If so then the current logic is sane > indeed; and if the current assumption in conversion routines is > contradicted then it must be due to a defect related to the kernel > driver. > Correct, if the kernel allocates (and thus we mmap) too small buffers for the current video fmt + dimensions then that is a kernel bug. <snip> Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c index 26a0978..46e6500 100644 --- a/lib/libv4lconvert/libv4lconvert.c +++ b/lib/libv4lconvert/libv4lconvert.c @@ -854,7 +854,7 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, if (src_size < (width * height)) { V4LCONVERT_ERR("short grey data frame\n"); errno = EPIPE; - result = -1; + return -1; } break; case V4L2_PIX_FMT_RGB565: And: diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c index 46e6500..a1a4858 100644 --- a/lib/libv4lconvert/libv4lconvert.c +++ b/lib/libv4lconvert/libv4lconvert.c @@ -841,6 +841,11 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, break; case V4L2_PIX_FMT_GREY: + if (src_size < (width * height)) { + V4LCONVERT_ERR("short grey data frame\n"); + errno = EPIPE; + return -1; + } switch (dest_pix_fmt) { case V4L2_PIX_FMT_RGB24: case V4L2_PIX_FMT_BGR24: @@ -851,11 +856,6 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, v4lconvert_grey_to_yuv420(src, dest, fmt); break; } - if (src_size < (width * height)) { - V4LCONVERT_ERR("short grey data frame\n"); - errno = EPIPE; - return -1; - } break; case V4L2_PIX_FMT_RGB565: switch (dest_pix_fmt) {