[media] tvp686x: Don't go past array

Depending on the compiler version, currently it produces the
following warnings:
	tw686x-video.c: In function 'tw686x_video_init':
	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]

This is actually bogus with the current code, as it currently
hardcodes the framerate to 30 frames/sec, however a potential
use after the array size could happen when the driver adds support
for setting the framerate. So, fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
  

On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]

I posted two patches fixing this and another issue:

https://patchwork.linuxtv.org/patch/33942/
https://patchwork.linuxtv.org/patch/33943/

I noticed that I accidentally set them to 'Accepted', so that might be
why you didn't see them.

I was planning on making a pull request for these on Monday, but you can
also take them now since Ezequiel already Acked them.

Regards,

	Hans

> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..1ff59084ce08 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_625_50) {
> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))
> +			i = 14;		/* 25 fps */
> +		else
> +			i = std_625_50[fps];
> +	} else {
> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> +			i = 0;		/* 30 fps */
> +		else
> +			i = std_525_60[fps];
> +	}
>  
>  	return map[i];
>  }
> 

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

Since my patch exchanges the sparse warning with a smatch warning, it's
OK to take this one, with a few corrections:

Please update the subject line (it says tvp686x instead of tw686x).

On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..1ff59084ce08 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_625_50) {

Please test against 525_60 since that is the recommended test.

> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))

Please don't use 'unlikely'. It's pointless for code that is rarely used.

Actually, the code is wrong: i is uninitialized here.

It should be fps >= ARRAY_SIZE(std_625_50).

In fact, I'd write it like this:

		i = std_625_50[(fps >= ARRAY_SIZE(std_625_50) ? 24 : fps];

> +			i = 14;		/* 25 fps */
> +		else
> +			i = std_625_50[fps];
> +	} else {
> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> +			i = 0;		/* 30 fps */
> +		else
> +			i = std_525_60[fps];
> +	}
>  
>  	return map[i];
>  }
> 

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