[PATCH 0/2] staging: rtl8188eu: fix Coverity defects in rtw_drv_init()

[PATCH 0/2] staging: rtl8188eu: fix Coverity defects in rtw_drv_init()

[Christian Engelmayer]

This addresses two issues currently reported by static analysis for function
rtw_drv_init() in drivers/staging/rtl8188eu/os_dep/usb_intf.c.

   CID 1077553 - Logically dead code
   CID 1077832 - Uninitialized scalar variable

Compile tested only. Applies against branch staging-next of tree
git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

Christian Engelmayer (2):
  staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()
  staging: rtl8188eu: remove dead code in rtw_drv_init()

 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/2] staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()

[Christian Engelmayer]

Function rtw_drv_init() is written in a way that assumes 'status' != _SUCCESS
as long as not explicitly set. Thus initialize 'status' to FAIL, in order to
prevent undefined behaviour if going through the exit paths. Detected by
Coverity - CID 1077832.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 8ed2ac8..632a5b0a 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -685,7 +685,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
 {
 	struct adapter *if1 = NULL;
-	int status;
+	int status = _FAIL;
 	struct dvobj_priv *dvobj;
 
 	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+rtw_drv_init\n"));
-- 
1.9.1

Re: [PATCH 1/2] staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()

[Dan Carpenter]

On Sat, May 17, 2014 at 12:38:57PM +0200, Christian Engelmayer wrote:
> Function rtw_drv_init() is written in a way that assumes 'status' != _SUCCESS
> as long as not explicitly set. Thus initialize 'status' to FAIL, in order to
> prevent undefined behaviour if going through the exit paths. Detected by
> Coverity - CID 1077832.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

This is a bugfix and we like to merge bugfixes without asking redo
things, so don't redo.  But really the better fix is to get rid of the
status variable completely.  Just return directly on the success path.

If we were to do that, then both patches would be merged together and
called:  [patch] Staging: rtl8188eu: fix error handling in rtw_drv_init()

But this patch is also acceptable as-is.  Thanks for fixing the bug.  :)

regards,
dan carpenter

Re: [PATCH 1/2] staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()

[Christian Engelmayer]

On Sat, 17 May 2014 17:44:23 +0300, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sat, May 17, 2014 at 12:38:57PM +0200, Christian Engelmayer wrote:
> > Function rtw_drv_init() is written in a way that assumes 'status' != _SUCCESS
> > as long as not explicitly set. Thus initialize 'status' to FAIL, in order to
> > prevent undefined behaviour if going through the exit paths. Detected by
> > Coverity - CID 1077832.
> > 
> > Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> 
> This is a bugfix and we like to merge bugfixes without asking redo
> things, so don't redo.  But really the better fix is to get rid of the
> status variable completely.  Just return directly on the success path.
> 
> If we were to do that, then both patches would be merged together and
> called:  [patch] Staging: rtl8188eu: fix error handling in rtw_drv_init()
> 
> But this patch is also acceptable as-is.  Thanks for fixing the bug.  :)

I agree with You Dan. I'm no big fan of that status variable either. In this
case I was already tempted, but saw it as a recurring pattern in that file
in case cleanup is done. So I decided to just attack the bug in a small change
and leave the cleanup of the error handling pattern for a later, consistent
sweep over the whole file if that's wanted.

Regards,
Christian

Re: [PATCH 1/2] staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()

[Dan Carpenter]

On Sat, May 17, 2014 at 08:56:35PM +0200, Christian Engelmayer wrote:
> On Sat, 17 May 2014 17:44:23 +0300, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Sat, May 17, 2014 at 12:38:57PM +0200, Christian Engelmayer wrote:
> > > Function rtw_drv_init() is written in a way that assumes 'status' != _SUCCESS
> > > as long as not explicitly set. Thus initialize 'status' to FAIL, in order to
> > > prevent undefined behaviour if going through the exit paths. Detected by
> > > Coverity - CID 1077832.
> > > 
> > > Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> > 
> > This is a bugfix and we like to merge bugfixes without asking redo
> > things, so don't redo.  But really the better fix is to get rid of the
> > status variable completely.  Just return directly on the success path.
> > 
> > If we were to do that, then both patches would be merged together and
> > called:  [patch] Staging: rtl8188eu: fix error handling in rtw_drv_init()
> > 
> > But this patch is also acceptable as-is.  Thanks for fixing the bug.  :)
> 
> I agree with You Dan. I'm no big fan of that status variable either. In this
> case I was already tempted, but saw it as a recurring pattern in that file
> in case cleanup is done. So I decided to just attack the bug in a small change
> and leave the cleanup of the error handling pattern for a later, consistent
> sweep over the whole file if that's wanted.

It's a mistake to try be consistent with the crap code in the rest of
this file.  ;)  Next time just fix it so at least a couple lines in here
are not terrible.

regards,
dan carpenter

[PATCH 2/2] staging: rtl8188eu: remove dead code in rtw_drv_init()

[Christian Engelmayer]

(status != _SUCCESS) immediately after 'status = _SUCCESS' will never evaluate
true. Thus remove the logically dead code. Detected by Coverity - CID 1077553.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 632a5b0a..2a96add 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -713,8 +713,6 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 
 	status = _SUCCESS;
 
-	if (status != _SUCCESS && if1)
-		rtw_usb_if1_deinit(if1);
 free_dvobj:
 	if (status != _SUCCESS)
 		usb_dvobj_deinit(pusb_intf);
-- 
1.9.1

Re: [PATCH 0/2] staging: rtl8188eu: fix Coverity defects in rtw_drv_init()

[Jes Sorensen]

Christian Engelmayer <cengelma@gmx.at> writes:
> This addresses two issues currently reported by static analysis for function
> rtw_drv_init() in drivers/staging/rtl8188eu/os_dep/usb_intf.c.
>
>    CID 1077553 - Logically dead code
>    CID 1077832 - Uninitialized scalar variable
>
> Compile tested only. Applies against branch staging-next of tree
> git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
>
> Christian Engelmayer (2):
>   staging: rtl8188eu: fix usage of uninit scalar in rtw_drv_init()
>   staging: rtl8188eu: remove dead code in rtw_drv_init()
>
>  drivers/staging/rtl8188eu/os_dep/usb_intf.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Acked-by: Jes Sorensen <Jes.Sorensen@redhat.com>