scsi: scsi_transport_iscsi.c initialize the variable before it get used

PATCH In review Jaswinder Singh Rajput 3 participants 6 messages May 26, 2009

Subsystems (2)

SCSI iSCSI

lore

Share & tools

lore

Jaswinder Singh Rajput

author

2009-05-26 15:19 UTC #0 lore raw

PATCH 1/1 scsi: scsi_transport_iscsi.c initialize the variable before it get used

I am watching this problem from long time in -tip.

Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used

In some cases, err will be used uninitialized.

Also fixed compilation warning :

CC drivers/scsi/scsi_transport_iscsi.o
drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function

Signed-off-by Jaswinder Singh Rajput <[email protected]>

---
 drivers/scsi/scsi_transport_iscsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/scsi/scsi_transport_iscsi.c +1 -1

index 0947954..e78cde2 100644
@@ -675,7 +675,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 	struct iscsi_cls_host *ihost;
 	unsigned long flags;
 	unsigned int id = target_id;
-	int err;
+	int err = 0;
 
 	ihost = shost->shost_data;
 	session->sid = atomic_add_return(1, &iscsi_session_nr);

Signature · 8 lines

1.6.0.6



--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2009-05-26 15:28 UTC #1 lore raw

Quoted 5 lines · Jaswinder Singh Rajput

I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.

What cases? A simple theoretical analysis shows that this variable is always set.

Quoted 4 lines

CC drivers/scsi/scsi_transport_iscsi.o
drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function

My version of gcc (4.3.3) correctly sees that the variable cannot be uninitialised ... what version are you using?

If it's a popular version, we can always do the uninitialised_var() thing, but if it's just a non-standard compiler, I'd rather not mess up the source code to please gcc.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jaswinder Singh Rajput

author

2009-05-26 16:00 UTC #2 lore raw

Hello James,

Quoted 10 lines · James Bottomley

Quoted 5 lines · Jaswinder Singh Rajput
I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.
What cases? A simple theoretical analysis shows that this variable is always set.

Ok lets takes your tree:

http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6

672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id) 673 {

674         struct Scsi_Host *shost = iscsi_session_to_shost(session);
675         struct iscsi_cls_host *ihost;
676         unsigned long flags;
677         unsigned int id = target_id;
678         int err;

679
680 ihost = shost->shost_data;
681 session->sid = atomic_add_return(1, &iscsi_session_nr);
682
683 if (id == ISCSI_MAX_TARGET) {
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
685 err = device_for_each_child(&shost->shost_gendev, &id,
686 iscsi_get_next_target_id);
687 if (!err)
688 break;
689 }
690
691 if (id == ISCSI_MAX_TARGET) {
692 iscsi_cls_session_printk(KERN_ERR, session,

693                                                  "Too many iscsi targets. Max "
694                                                  "number of targets is %d.\n",
695                                                  ISCSI_MAX_TARGET - 1);

696 goto release_host;
697 }
698 }
699 session->target_id = id;
700

701         dev_set_name(&session->dev, "session%u", session->sid);
702         err = device_add(&session->dev);
703         if (err) {

704 iscsi_cls_session_printk(KERN_ERR, session,
705 "could not register session's dev\n");
706 goto release_host;
707 }
708 transport_register_device(&session->dev);
709

710         spin_lock_irqsave(&sesslock, flags);
711         list_add(&session->sess_list, &sesslist);
712         spin_unlock_irqrestore(&sesslock, flags);

713
714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
715 return 0;
716
717 release_host:
718 scsi_host_put(shost);
719 return err;
720 }

You are right but compiler is confused and giving warning like this :

1. do not go inside :
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {

2. and then choose path :
696 goto release_host;

then we return err uninitialized ;-)

Quoted 5 lines

Quoted 4 lines
CC drivers/scsi/scsi_transport_iscsi.o
drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function

Quoted 7 lines

My version of gcc (4.3.3) correctly sees that the variable cannot be uninitialised ... what version are you using?
If it's a popular version, we can always do the uninitialised_var() thing, but if it's just a non-standard compiler, I'd rather not mess up the source code to please gcc.

gcc version 4.3.2 20081105 (Red Hat 4.3.2-7) (GCC)

--
JSR

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2009-05-26 16:06 UTC #3 lore raw

Quoted 69 lines · Jaswinder Singh Rajput

Hello James,
Quoted 10 lines · James Bottomley
Quoted 5 lines · Jaswinder Singh Rajput
I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.
What cases? A simple theoretical analysis shows that this variable is always set.
Ok lets takes your tree:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6
672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id) 673 {
674         struct Scsi_Host *shost = iscsi_session_to_shost(session);
675         struct iscsi_cls_host *ihost;
676         unsigned long flags;
677         unsigned int id = target_id;
678         int err;
679
680 ihost = shost->shost_data;
681 session->sid = atomic_add_return(1, &iscsi_session_nr);
682
683 if (id == ISCSI_MAX_TARGET) {
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
685 err = device_for_each_child(&shost->shost_gendev, &id,
686 iscsi_get_next_target_id);
687 if (!err)
688 break;
689 }
690
691 if (id == ISCSI_MAX_TARGET) {
692 iscsi_cls_session_printk(KERN_ERR, session,
693                                                  "Too many iscsi targets. Max "
694                                                  "number of targets is %d.\n",
695                                                  ISCSI_MAX_TARGET - 1);
696 goto release_host;
697 }
698 }
699 session->target_id = id;
700
701         dev_set_name(&session->dev, "session%u", session->sid);
702         err = device_add(&session->dev);
703         if (err) {
704 iscsi_cls_session_printk(KERN_ERR, session,
705 "could not register session's dev\n");
706 goto release_host;
707 }
708 transport_register_device(&session->dev);
709
710         spin_lock_irqsave(&sesslock, flags);
711         list_add(&session->sess_list, &sesslist);
712         spin_unlock_irqrestore(&sesslock, flags);
713
714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
715 return 0;
716
717 release_host:
718 scsi_host_put(shost);
719 return err;
720 }

Quoted 4 lines

You are right but compiler is confused and giving warning like this :
1. do not go inside :
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {

How? id is an unsigned int and ISCSI_MAX_TARGET is defined to be -1 in include/scsi/scsi_transport_iscsi.h (might be better if it were UINT_MAX, but -1 is just as good).

James

Jaswinder Singh Rajput

author

2009-05-26 16:21 UTC #4 lore raw

Quoted 70 lines · James Bottomley

Quoted 69 lines · Jaswinder Singh Rajput
Hello James,
Quoted 10 lines · James Bottomley
Quoted 5 lines · Jaswinder Singh Rajput
I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.
What cases? A simple theoretical analysis shows that this variable is always set.
Ok lets takes your tree:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6
672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id) 673 {
674         struct Scsi_Host *shost = iscsi_session_to_shost(session);
675         struct iscsi_cls_host *ihost;
676         unsigned long flags;
677         unsigned int id = target_id;
678         int err;
679
680 ihost = shost->shost_data;
681 session->sid = atomic_add_return(1, &iscsi_session_nr);
682
683 if (id == ISCSI_MAX_TARGET) {
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
685 err = device_for_each_child(&shost->shost_gendev, &id,
686 iscsi_get_next_target_id);
687 if (!err)
688 break;
689 }
690
691 if (id == ISCSI_MAX_TARGET) {
692 iscsi_cls_session_printk(KERN_ERR, session,
693                                                  "Too many iscsi targets. Max "
694                                                  "number of targets is %d.\n",
695                                                  ISCSI_MAX_TARGET - 1);
696 goto release_host;
697 }
698 }
699 session->target_id = id;
700
701         dev_set_name(&session->dev, "session%u", session->sid);
702         err = device_add(&session->dev);
703         if (err) {
704 iscsi_cls_session_printk(KERN_ERR, session,
705 "could not register session's dev\n");
706 goto release_host;
707 }
708 transport_register_device(&session->dev);
709
710         spin_lock_irqsave(&sesslock, flags);
711         list_add(&session->sess_list, &sesslist);
712         spin_unlock_irqrestore(&sesslock, flags);
713
714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
715 return 0;
716
717 release_host:
718 scsi_host_put(shost);
719 return err;
720 }

Quoted 9 lines

Quoted 4 lines
You are right but compiler is confused and giving warning like this :
1. do not go inside :
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
How? id is an unsigned int and ISCSI_MAX_TARGET is defined to be -1 in include/scsi/scsi_transport_iscsi.h (might be better if it were UINT_MAX, but -1 is just as good).

Yes, you are right, I was telling you how compiler was confused in 4.3.2 but I am glad to hear that this problem is fixed in 4.3.3 as you told me :-)

So now its upto you. 4.3.2 is not soo old, So either initialize the variable or stay with stray warning in the kernel ;-)

Thanks,
--
JSR

Mike Christie

2009-05-26 21:42 UTC #5 lore raw

In reply to Jaswinder Singh Rajput

Quoted 6 lines · Jaswinder Singh Rajput

I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.

As you guys already discussed it will not get used uninitialized.

Quoted 6 lines

Also fixed compilation warning :
CC drivers/scsi/scsi_transport_iscsi.o
drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function

I did the attached patch to silence the compile warning. I goofed and forgot to send it before, because I had change my compiler and was not seeing the error message. I was planning on sending the attached with some other changes for the next feature window. I am going to fix up the patch to use UINT_MAX like James mentioned in the thread.

Quoted 1 line

From 32ffa5248bf11f4df76ebf3c4a023958a9131f9c Mon Sep 17 00:00:00 2001

From: Mike Christie <[email protected]>
Date: Mon, 18 May 2009 11:54:37 -0500
Subject: [PATCH] iscsi class: quiet iscsi_add_session compilation warning.

If ISCSI_MAX_TARGET was zero then iscsi_add_session could return success when it had failed. This will currently not happen because ISCSI_MAX_TARGET is a macro and not a variable. But gcc likes to complain about it and people keep reporting it. This patch just initializes err to be -EINVAL so if we ever defined ISCSI_MAX_TARGET incorrectly we would fail gracefully.

---
 drivers/scsi/scsi_transport_iscsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/scsi/scsi_transport_iscsi.c +1 -1

index 0a2ce7b..f508567 100644
@@ -675,7 +675,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 	struct iscsi_cls_host *ihost;
 	unsigned long flags;
 	unsigned int id = target_id;
-	int err;
+	int err = -EINVAL;
 
 	ihost = shost->shost_data;
 	session->sid = atomic_add_return(1, &iscsi_session_nr);

Signature · 1 line

1.6.0.6