Skip to contentSkip to conversation

drivers/media/video: move dereference after NULL test

PATCH Applied [email protected] 3 participants 6 messages Mar 11, 2010 – Mar 12, 2010 Media (V4L/DVB)

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 2/5 drivers/media/video: move dereference after NULL test

From: Julia Lawall <[email protected]>

In quickcam_messenger.c, if the NULL test on uvd is needed, then the dereference should be after the NULL test.

In vpif_display.c, std_info is initialized to the address of a structure field. This seems unlikely to be NULL. If it could somehow be NULL, then the assignment should be moved after the NULL test. Alternatively, perhaps the NULL test is intended to test std_info->stdid rather than std_info?

In saa7134-alsa.c, the function is only called from one place, where the chip argument has already been dereferenced. On the other hand, if it should be kept, then card should be initialized after it.

A simplified version of the semantic match that detects this problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
@match exists@
expression x, E;
identifier fld;
@@

* x->fld
... when != \(x = E\|&x\)
* x == NULL
// </smpl>

Signed-off-by Julia Lawall <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 drivers/media/video/davinci/vpif_display.c        |    2 --
 drivers/media/video/saa7134/saa7134-alsa.c        |    2 --
 drivers/media/video/usbvideo/quickcam_messenger.c |    3 ++-
 3 files changed, 2 insertions(+), 5 deletions(-)
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -2
diff -puN drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test drivers/media/video/davinci/vpif_display.c
@@ -383,8 +383,6 @@ static int vpif_get_std_info(struct chan
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
-		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
 		config = &ch_params[index];
drivers/media/video/saa7134/saa7134-alsa.c renamed from drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move-dereference-after-null-test +0 -2
diff -puN drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move-dereference-after-null-test drivers/media/video/saa7134/saa7134-alsa.c
@@ -1011,8 +1011,6 @@ static int snd_card_saa7134_new_mixer(sn
 	unsigned int idx;
 	int err, addr;
 
-	if (snd_BUG_ON(!chip))
-		return -EINVAL;
 	strcpy(card->mixername, "SAA7134 Mixer");
 
 	for (idx = 0; idx < ARRAY_SIZE(snd_saa7134_volume_controls); idx++) {
drivers/media/video/usbvideo/quickcam_messenger.c renamed from drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media-video-move-dereference-after-null-test +2 -1
diff -puN drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media-video-move-dereference-after-null-test drivers/media/video/usbvideo/quickcam_messenger.c
@@ -692,12 +692,13 @@ static int qcm_start_data(struct uvd *uv
 
 static void qcm_stop_data(struct uvd *uvd)
 {
-	struct qcm *cam = (struct qcm *) uvd->user_data;
+	struct qcm *cam;
 	int i, j;
 	int ret;
 
 	if ((uvd == NULL) || (!uvd->streaming) || (uvd->dev == NULL))
 		return;
+	cam = (struct qcm *) uvd->user_data;
 
 	ret = qcm_camera_off(uvd);
 	if (ret)

_

#1 lore raw
Quoted 53 lines

-----Original Message-----
From: [email protected] [mailto:linux-media- [email protected]] On Behalf Of [email protected]
Sent: Thursday, March 11, 2010 5:02 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: [patch 2/5] drivers/media/video: move dereference after NULL test

From: Julia Lawall <[email protected]>

In quickcam_messenger.c, if the NULL test on uvd is needed, then the dereference should be after the NULL test.

In vpif_display.c, std_info is initialized to the address of a structure field. This seems unlikely to be NULL. If it could somehow be NULL, then the assignment should be moved after the NULL test. Alternatively, perhaps the NULL test is intended to test std_info->stdid rather than std_info?

In saa7134-alsa.c, the function is only called from one place, where the chip argument has already been dereferenced. On the other hand, if it should be kept, then card should be initialized after it.

A simplified version of the semantic match that detects this problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
@match exists@
expression x, E;
identifier fld;
@@

* x->fld
... when != \(x = E\|&x\)
* x == NULL
// </smpl>

Signed-off-by Julia Lawall <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

drivers/media/video/davinci/vpif_display.c        |    2 --
drivers/media/video/saa7134/saa7134-alsa.c        |    2 --
drivers/media/video/usbvideo/quickcam_messenger.c |    3 ++-
3 files changed, 2 insertions(+), 5 deletions(-)

diff -puN drivers/media/video/davinci/vpif_display.c~drivers-media-video- move-dereference-after-null-test drivers/media/video/davinci/vpif_display.c --- a/drivers/media/video/davinci/vpif_display.c~drivers-media-video-move- dereference-after-null-test
+++ a/drivers/media/video/davinci/vpif_display.c

drivers/media/video/davinci/vpif_display.c static int vpif_get_std_info(struct chan · line 387

Quoted 5 lines
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -2
@@ -383,8 +383,6 @@ static int vpif_get_std_info(struct chan
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
-		return -1;

Please change it as

if (!std_info->stdid)

	return -1;

Murali

Quoted 43 lines
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -0
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
 		config = &ch_params[index];

diff -puN drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video- move-dereference-after-null-test drivers/media/video/saa7134/saa7134-alsa.c --- a/drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move- dereference-after-null-test
+++ a/drivers/media/video/saa7134/saa7134-alsa.c

drivers/media/video/saa7134/saa7134-alsa.c renamed from drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move-dereference-after-null-test +0 -2
@@ -1011,8 +1011,6 @@ static int snd_card_saa7134_new_mixer(sn
 	unsigned int idx;
 	int err, addr;
 
-	if (snd_BUG_ON(!chip))
-		return -EINVAL;
 	strcpy(card->mixername, "SAA7134 Mixer");
 
 	for (idx = 0; idx < ARRAY_SIZE(snd_saa7134_volume_controls); idx++) {

diff -puN drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media- video-move-dereference-after-null-test drivers/media/video/usbvideo/quickcam_messenger.c
--- a/drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media- video-move-dereference-after-null-test
+++ a/drivers/media/video/usbvideo/quickcam_messenger.c

drivers/media/video/usbvideo/quickcam_messenger.c renamed from drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media-video-move-dereference-after-null-test +2 -1
@@ -692,12 +692,13 @@ static int qcm_start_data(struct uvd *uv
 
 static void qcm_stop_data(struct uvd *uvd)
 {
-	struct qcm *cam = (struct qcm *) uvd->user_data;
+	struct qcm *cam;
 	int i, j;
 	int ret;
 
 	if ((uvd == NULL) || (!uvd->streaming) || (uvd->dev == NULL))
 		return;
+	cam = (struct qcm *) uvd->user_data;
 
 	ret = qcm_camera_off(uvd);
 	if (ret)

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

author
#2 lore raw
Quoted 8 lines · Muralidharan
Quoted 8 lines

diff -puN drivers/media/video/davinci/vpif_display.c~drivers-media-video- move-dereference-after-null-test drivers/media/video/davinci/vpif_display.c --- a/drivers/media/video/davinci/vpif_display.c~drivers-media-video-move- dereference-after-null-test
+++ a/drivers/media/video/davinci/vpif_display.c

drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -0
@@ -383,8 +383,6 @@ static int vpif_get_std_info(struct chan
 	int index;
Quoted 8 lines

drivers/media/video/davinci/vpif_display.c static int vpif_get_std_info(struct chan · line 387

Quoted 3 lines
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -2
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
-		return -1;

Please change it as

if (!std_info->stdid)

	return -1;

Could you please do this, and send the patch? It's better that way as you're more familar with the code and maybe can even test it.

#3 lore raw
In reply to Karicheri, Muralidharan

Oops, my mistake. I will fix that.

julia

Quoted 109 lines · Muralidharan
Quoted 53 lines

-----Original Message-----
From: [email protected] [mailto:linux-media- [email protected]] On Behalf Of [email protected]
Sent: Thursday, March 11, 2010 5:02 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: [patch 2/5] drivers/media/video: move dereference after NULL test

From: Julia Lawall <[email protected]>

In quickcam_messenger.c, if the NULL test on uvd is needed, then the dereference should be after the NULL test.

In vpif_display.c, std_info is initialized to the address of a structure field. This seems unlikely to be NULL. If it could somehow be NULL, then the assignment should be moved after the NULL test. Alternatively, perhaps the NULL test is intended to test std_info->stdid rather than std_info?

In saa7134-alsa.c, the function is only called from one place, where the chip argument has already been dereferenced. On the other hand, if it should be kept, then card should be initialized after it.

A simplified version of the semantic match that detects this problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
@match exists@
expression x, E;
identifier fld;
@@

* x->fld
... when != \(x = E\|&x\)
* x == NULL
// </smpl>

Signed-off-by Julia Lawall <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

drivers/media/video/davinci/vpif_display.c        |    2 --
drivers/media/video/saa7134/saa7134-alsa.c        |    2 --
drivers/media/video/usbvideo/quickcam_messenger.c |    3 ++-
3 files changed, 2 insertions(+), 5 deletions(-)

diff -puN drivers/media/video/davinci/vpif_display.c~drivers-media-video- move-dereference-after-null-test drivers/media/video/davinci/vpif_display.c --- a/drivers/media/video/davinci/vpif_display.c~drivers-media-video-move- dereference-after-null-test
+++ a/drivers/media/video/davinci/vpif_display.c

drivers/media/video/davinci/vpif_display.c static int vpif_get_std_info(struct chan · line 387

Quoted 5 lines
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -2
@@ -383,8 +383,6 @@ static int vpif_get_std_info(struct chan
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
-		return -1;

Please change it as

if (!std_info->stdid)

	return -1;

Murali

Quoted 43 lines
drivers/media/video/davinci/vpif_display.c renamed from drivers/media/video/davinci/vpif_display.c~drivers-media-video-move-dereference-after-null-test +0 -0
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
 		config = &ch_params[index];

diff -puN drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video- move-dereference-after-null-test drivers/media/video/saa7134/saa7134-alsa.c --- a/drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move- dereference-after-null-test
+++ a/drivers/media/video/saa7134/saa7134-alsa.c

drivers/media/video/saa7134/saa7134-alsa.c renamed from drivers/media/video/saa7134/saa7134-alsa.c~drivers-media-video-move-dereference-after-null-test +0 -2
@@ -1011,8 +1011,6 @@ static int snd_card_saa7134_new_mixer(sn
 	unsigned int idx;
 	int err, addr;
 
-	if (snd_BUG_ON(!chip))
-		return -EINVAL;
 	strcpy(card->mixername, "SAA7134 Mixer");
 
 	for (idx = 0; idx < ARRAY_SIZE(snd_saa7134_volume_controls); idx++) {

diff -puN drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media- video-move-dereference-after-null-test drivers/media/video/usbvideo/quickcam_messenger.c
--- a/drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media- video-move-dereference-after-null-test
+++ a/drivers/media/video/usbvideo/quickcam_messenger.c

drivers/media/video/usbvideo/quickcam_messenger.c renamed from drivers/media/video/usbvideo/quickcam_messenger.c~drivers-media-video-move-dereference-after-null-test +2 -1
@@ -692,12 +692,13 @@ static int qcm_start_data(struct uvd *uv
 
 static void qcm_stop_data(struct uvd *uvd)
 {
-	struct qcm *cam = (struct qcm *) uvd->user_data;
+	struct qcm *cam;
 	int i, j;
 	int ret;
 
 	if ((uvd == NULL) || (!uvd->streaming) || (uvd->dev == NULL))
 		return;
+	cam = (struct qcm *) uvd->user_data;
 
 	ret = qcm_camera_off(uvd);
 	if (ret)

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

#4 lore raw

From: Julia Lawall <[email protected]>

In quickcam_messenger.c, if the NULL test on uvd is needed, then the dereference should be after the NULL test.

In vpif_display.c, std_info is initialized to the address of a structure field. This seems unlikely to be NULL. Test std_info->stdid instead.

In saa7134-alsa.c, the function is only called from one place, where the chip argument has already been dereferenced. On the other hand, if it should be kept, then card should be initialized after it.

A simplified version of the semantic match that detects this problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
@match exists@
expression x, E;
identifier fld;
@@

* x->fld
... when != \(x = E\|&x\)
* x == NULL
// </smpl>

Signed-off-by Julia Lawall <[email protected]>
---
 drivers/media/video/davinci/vpif_display.c        |    2 +-
 drivers/media/video/saa7134/saa7134-alsa.c        |    2 --
 drivers/media/video/usbvideo/quickcam_messenger.c |    3 ++-
 3 files changed, 3 insertions(+), 4 deletions(-)
drivers/media/video/usbvideo/quickcam_messenger.c +2 -1
index 803d3e4..f0043d0 100644
@@ -692,12 +692,13 @@ static int qcm_start_data(struct uvd *uvd)
 
 static void qcm_stop_data(struct uvd *uvd)
 {
-	struct qcm *cam = (struct qcm *) uvd->user_data;
+	struct qcm *cam;
 	int i, j;
 	int ret;
 
 	if ((uvd == NULL) || (!uvd->streaming) || (uvd->dev == NULL))
 		return;
+	cam = (struct qcm *) uvd->user_data;
 
 	ret = qcm_camera_off(uvd);
 	if (ret)
drivers/media/video/saa7134/saa7134-alsa.c +0 -2
index d48c450..d3bd82a 100644
@@ -1011,8 +1011,6 @@ static int snd_card_saa7134_new_mixer(snd_card_saa7134_t * chip)
 	unsigned int idx;
 	int err, addr;
 
-	if (snd_BUG_ON(!chip))
-		return -EINVAL;
 	strcpy(card->mixername, "SAA7134 Mixer");
 
 	for (idx = 0; idx < ARRAY_SIZE(snd_saa7134_volume_controls); idx++) {
drivers/media/video/davinci/vpif_display.c +1 -1
index dfddef7..b2dce78 100644
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
Acked-by
#5 lore raw

For drivers/media/video/davinci/vpif_display.c

Acked-by Muralidharan Karicheri <[email protected]>

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: [email protected]

Quoted 92 lines

-----Original Message-----
From: Julia Lawall [mailto:[email protected]]
Sent: Friday, March 12, 2010 4:16 AM
To: Karicheri, Muralidharan
Cc: [email protected]; [email protected]; linux- [email protected]
Subject: RE: [patch 2/5] drivers/media/video: move dereference after NULL test

From: Julia Lawall <[email protected]>

In quickcam_messenger.c, if the NULL test on uvd is needed, then the dereference should be after the NULL test.

In vpif_display.c, std_info is initialized to the address of a structure field. This seems unlikely to be NULL. Test std_info->stdid instead.

In saa7134-alsa.c, the function is only called from one place, where the chip argument has already been dereferenced. On the other hand, if it should be kept, then card should be initialized after it.

A simplified version of the semantic match that detects this problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
@match exists@
expression x, E;
identifier fld;
@@

* x->fld
... when != \(x = E\|&x\)
* x == NULL
// </smpl>

Signed-off-by Julia Lawall <[email protected]>

---

drivers/media/video/davinci/vpif_display.c        |    2 +-
drivers/media/video/saa7134/saa7134-alsa.c        |    2 --
drivers/media/video/usbvideo/quickcam_messenger.c |    3 ++-
3 files changed, 3 insertions(+), 4 deletions(-)
drivers/media/video/usbvideo/quickcam_messenger.c +2 -1
index 803d3e4..f0043d0 100644
@@ -692,12 +692,13 @@ static int qcm_start_data(struct uvd *uvd)
 
 static void qcm_stop_data(struct uvd *uvd)
 {
-	struct qcm *cam = (struct qcm *) uvd->user_data;
+	struct qcm *cam;
 	int i, j;
 	int ret;
 
 	if ((uvd == NULL) || (!uvd->streaming) || (uvd->dev == NULL))
 		return;
+	cam = (struct qcm *) uvd->user_data;
 
 	ret = qcm_camera_off(uvd);
 	if (ret)
drivers/media/video/saa7134/saa7134-alsa.c +0 -2
index d48c450..d3bd82a 100644
@@ -1011,8 +1011,6 @@ static int
 snd_card_saa7134_new_mixer(snd_card_saa7134_t * chip)
 	unsigned int idx;
 	int err, addr;
 
-	if (snd_BUG_ON(!chip))
-		return -EINVAL;
 	strcpy(card->mixername, "SAA7134 Mixer");
 

	for (idx = 0; idx < ARRAY_SIZE(snd_saa7134_volume_controls); idx++) {
drivers/media/video/davinci/vpif_display.c +1 -1
index dfddef7..b2dce78 100644
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {