Question about libv4lconvert.

[Antonio Ospite <ospite@xxxxxxxxxxxxxxxxx>]

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: pgpqV427soM58.pgp
Description: PGP signature