Bttv: move check on unsigned

PATCH Trent Piepho 2 participants 3 messages Jan 19, 2009 – Jan 29, 2009 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

Trent Piepho

author

2009-01-19 18:21 UTC #1 lore raw

In reply to a message not in the archive

Quoted 10 lines · Roel Kluin

Please review, this patch was not tested.
The static function set_tvnorm is called in
drivers/media/video/bt8xx/bttv-driver.c:
1355:   set_tvnorm(btv, norm);
1868:   set_tvnorm(btv, i);
3273:   set_tvnorm(btv,btv->tvnorm);
in the first two with an unsigned, but bttv->tvnorm is signed.

Probably better to just change bttv->tvnorm is unsigned if we can.

Quoted 38 lines

see vi drivers/media/video/bt8xx/bttvp.h +381
since norm is unsigned in set_tvnorm, a negative won't get noticed. so remove the redundant check and move it to the caller.
My question is: should we error return like this?
Signed-off-by Roel Kluin <[email protected]>
---
drivers/media/video/bt8xx/bttv-driver.c +5 -2
index c71f394..6f50f90 100644
@@ -1290,7 +1290,7 @@ set_tvnorm(struct bttv *btv, unsigned int norm)
 	const struct bttv_tvnorm *tvnorm;
 	v4l2_std_id id;
 
-	if (norm < 0 || norm >= BTTV_TVNORMS)
+	if (norm >= BTTV_TVNORMS)
 		return -EINVAL;
 
 	tvnorm = &bttv_tvnorms[norm];
@@ -3266,6 +3266,10 @@ static int bttv_open(struct file *file)
 			    V4L2_FIELD_SEQ_TB,
 			    sizeof(struct bttv_buffer),
 			    fh);
+	if (btv->norm < 0) {
+                unlock_kernel();
+                return -EINVAL;
+        }
 	set_tvnorm(btv,btv->tvnorm);
 	set_input(btv, btv->input, btv->tvnorm);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Trent Piepho

author

2009-01-25 01:27 UTC #2 lore raw

Quoted 6 lines · Trent Piepho

Quoted 5 lines · Roel Kluin
Please review, this patch was not tested.
The static function set_tvnorm is called in
drivers/media/video/bt8xx/bttv-driver.c:

Quoted 7 lines

Quoted 5 lines
1355:   set_tvnorm(btv, norm);
1868:   set_tvnorm(btv, i);
3273:   set_tvnorm(btv,btv->tvnorm);
in the first two with an unsigned, but bttv->tvnorm is signed.
Probably better to just change bttv->tvnorm is unsigned if we can.

Here is an improved patch that does a full tvnorm fix for the driver. The tvnorm value is an index into an array and is never allowed to be negative or otherwise invalid. Most places it was passed around were unsigned, but a few structs and functions had signed values.

I got rid of the "< 0" checks and changed some ">= BTTV_TVNORMS" checks to BUG_ON().

Any problems with this patch Roel?

Mauro, don't apply as is, I'll send a pull request for a real patch later.

linux/drivers/media/video/bt8xx/bttv-cards.c +1 -1

diff -r 6a6eb9efc6cd linux/drivers/media/video/bt8xx/bttv-cards.c
@@ -4131,7 +4131,7 @@ static void __devinit avermedia_eeprom(s
 }
 
 /* used on Voodoo TV/FM (Voodoo 200), S0 wired to 0x10000 */
-void bttv_tda9880_setnorm(struct bttv *btv, int norm)
+void bttv_tda9880_setnorm(struct bttv *btv, unsigned int norm)
 {
 	/* fix up our card entry */
 	if(norm==V4L2_STD_NTSC) {

linux/drivers/media/video/bt8xx/bttv-driver.c +5 -8

diff -r 6a6eb9efc6cd linux/drivers/media/video/bt8xx/bttv-driver.c
@@ -1300,7 +1300,7 @@ bttv_crop_calc_limits(struct bttv_crop *
 }
 
 static void
-bttv_crop_reset(struct bttv_crop *c, int norm)
+bttv_crop_reset(struct bttv_crop *c, unsigned int norm)
 {
 	c->rect = bttv_tvnorms[norm].cropcap.defrect;
 	bttv_crop_calc_limits(c);

@@ -1313,16 +1313,13 @@ set_tvnorm(struct bttv *btv, unsigned in
 	const struct bttv_tvnorm *tvnorm;
 	v4l2_std_id id;
 
-	if (norm < 0 || norm >= BTTV_TVNORMS)
-		return -EINVAL;
+	BUG_ON(norm >= BTTV_TVNORMS);
+	BUG_ON(btv->tvnorm >= BTTV_TVNORMS);
 
 	tvnorm = &bttv_tvnorms[norm];
 
-	if (btv->tvnorm < 0 ||
-	    btv->tvnorm >= BTTV_TVNORMS ||
-	    0 != memcmp(&bttv_tvnorms[btv->tvnorm].cropcap,
-			&tvnorm->cropcap,
-			sizeof (tvnorm->cropcap))) {
+	if (!memcmp(&bttv_tvnorms[btv->tvnorm].cropcap, &tvnorm->cropcap,
+		    sizeof (tvnorm->cropcap))) {
 		bttv_crop_reset(&btv->crop[0], norm);
 		btv->crop[1] = btv->crop[0]; /* current = default */

linux/drivers/media/video/bt8xx/bttv-vbi.c +1 -1

diff -r 6a6eb9efc6cd linux/drivers/media/video/bt8xx/bttv-vbi.c
@@ -411,7 +411,7 @@ int bttv_g_fmt_vbi_cap(struct file *file
 	return 0;
 }
 
-void bttv_vbi_fmt_reset(struct bttv_vbi_fmt *f, int norm)
+void bttv_vbi_fmt_reset(struct bttv_vbi_fmt *f, unsigned int norm)
 {
 	const struct bttv_tvnorm *tvnorm;
 	unsigned int real_samples_per_line;

linux/drivers/media/video/bt8xx/bttv.h +1 -1

diff -r 6a6eb9efc6cd linux/drivers/media/video/bt8xx/bttv.h
@@ -266,7 +266,7 @@ extern void bttv_init_card2(struct bttv
 
 /* card-specific funtions */
 extern void tea5757_set_freq(struct bttv *btv, unsigned short freq);
-extern void bttv_tda9880_setnorm(struct bttv *btv, int norm);
+extern void bttv_tda9880_setnorm(struct bttv *btv, unsigned int norm);
 
 /* extra tweaks for some chipsets */
 extern void bttv_check_chipset(void);

linux/drivers/media/video/bt8xx/bttvp.h +5 -4

diff -r 6a6eb9efc6cd linux/drivers/media/video/bt8xx/bttvp.h
@@ -137,7 +137,7 @@ struct bttv_buffer {
 
 	/* bttv specific */
 	const struct bttv_format   *fmt;
-	int                        tvnorm;
+	unsigned int               tvnorm;
 	int                        btformat;
 	int                        btswap;
 	struct bttv_geometry       geo;

@@ -156,7 +156,7 @@ struct bttv_buffer_set {
 };
 
 struct bttv_overlay {
-	int                    tvnorm;
+	unsigned int           tvnorm;
 	struct v4l2_rect       w;
 	enum v4l2_field        field;
 	struct v4l2_clip       *clips;

@@ -176,7 +176,7 @@ struct bttv_vbi_fmt {
 };
 
 /* bttv-vbi.c */
-void bttv_vbi_fmt_reset(struct bttv_vbi_fmt *f, int norm);
+void bttv_vbi_fmt_reset(struct bttv_vbi_fmt *f, unsigned int norm);
 
 struct bttv_crop {
 	/* A cropping rectangle in struct bttv_tvnorm.cropcap units. */

@@ -380,7 +380,8 @@ struct bttv {
 	unsigned int audio;
 	unsigned int mute;
 	unsigned long freq;
-	int tvnorm,hue,contrast,bright,saturation;
+	unsigned int tvnorm;
+	int hue, contrast, bright, saturation;
 	struct v4l2_framebuffer fbuf;
 	unsigned int field_count;

Mauro Carvalho Chehab

2009-01-29 09:19 UTC #3 lore raw

Quoted 20 lines · On Sat, 24 Jan 2009 17:27:09 -0800 (PST) Trent Piepho <[email protected]>…

Quoted 6 lines · Trent Piepho
Quoted 5 lines · Roel Kluin
Please review, this patch was not tested.
The static function set_tvnorm is called in
drivers/media/video/bt8xx/bttv-driver.c:
Quoted 7 lines
Quoted 5 lines
1355:   set_tvnorm(btv, norm);
1868:   set_tvnorm(btv, i);
3273:   set_tvnorm(btv,btv->tvnorm);
in the first two with an unsigned, but bttv->tvnorm is signed.
Probably better to just change bttv->tvnorm is unsigned if we can.
Here is an improved patch that does a full tvnorm fix for the driver. The tvnorm value is an index into an array and is never allowed to be negative or otherwise invalid. Most places it was passed around were unsigned, but a few structs and functions had signed values.

Quoted 6 lines

I got rid of the "< 0" checks and changed some ">= BTTV_TVNORMS" checks to BUG_ON().
Any problems with this patch Roel?
Mauro, don't apply as is, I'll send a pull request for a real patch later.

Ok.

Cheers,
Mauro