Question about libv4lconvert.

Question about libv4lconvert.

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

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:

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) {


Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: Question about libv4lconvert.

[Hans de Goede]

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.

Re: Question about libv4lconvert.

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 3750 bytes --]

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.


-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: Question about libv4lconvert.

[Hans de Goede]

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