[PATCH] tm6000: fix vbuf may be used uninitialized

[PATCH] tm6000: fix vbuf may be used uninitialized

[Jarod Wilson]

Signed-off-by: Jarod Wilson <jarod@redhat.com>
CC: devel@driverdev.osuosl.org
---
 drivers/staging/tm6000/tm6000-video.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
index c80a316..bfebedd 100644
--- a/drivers/staging/tm6000/tm6000-video.c
+++ b/drivers/staging/tm6000/tm6000-video.c
@@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long len,
 	unsigned long header = 0;
 	int rc = 0;
 	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
-	struct tm6000_buffer *vbuf;
+	struct tm6000_buffer *vbuf = NULL;
 	char *voutp = NULL;
 	unsigned int linewidth;
 
-- 
1.7.1

Re: [PATCH] tm6000: fix vbuf may be used uninitialized

[Dan Carpenter]

On Thu, Mar 24, 2011 at 04:07:00PM -0400, Jarod Wilson wrote:
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Jarod, there is a lot of information missing from your change log...  :/

> CC: devel@driverdev.osuosl.org
> ---
>  drivers/staging/tm6000/tm6000-video.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
> index c80a316..bfebedd 100644
> --- a/drivers/staging/tm6000/tm6000-video.c
> +++ b/drivers/staging/tm6000/tm6000-video.c
> @@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long len,
>  	unsigned long header = 0;
>  	int rc = 0;
>  	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
> -	struct tm6000_buffer *vbuf;
> +	struct tm6000_buffer *vbuf = NULL;
>  	char *voutp = NULL;
>  	unsigned int linewidth;
>  

This looks like a real bug versus just a GCC warning.  It was introduced
in 8aff8ba95155df "[media] tm6000: add radio support to the driver".
I've added Dmitri to the CC list.

regards,
dan carpenter

Re: [PATCH] tm6000: fix vbuf may be used uninitialized

[Jarod Wilson]

On Thu, Mar 24, 2011 at 11:31:08PM +0300, Dan Carpenter wrote:
> On Thu, Mar 24, 2011 at 04:07:00PM -0400, Jarod Wilson wrote:
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> 
> Jarod, there is a lot of information missing from your change log...  :/

Hrm, I'm building the media stack with all warnings fatal, so this was
just a quick fix to silence the compiler warning, didn't really look into
it at all.

> > CC: devel@driverdev.osuosl.org
> > ---
> >  drivers/staging/tm6000/tm6000-video.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
> > index c80a316..bfebedd 100644
> > --- a/drivers/staging/tm6000/tm6000-video.c
> > +++ b/drivers/staging/tm6000/tm6000-video.c
> > @@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long len,
> >  	unsigned long header = 0;
> >  	int rc = 0;
> >  	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
> > -	struct tm6000_buffer *vbuf;
> > +	struct tm6000_buffer *vbuf = NULL;
> >  	char *voutp = NULL;
> >  	unsigned int linewidth;
> >  
> 
> This looks like a real bug versus just a GCC warning.  It was introduced
> in 8aff8ba95155df "[media] tm6000: add radio support to the driver".
> I've added Dmitri to the CC list.

Thanks much, will try to pay more attention next time. ;)

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] tm6000: fix vbuf may be used uninitialized

[Jarod Wilson]

On Mar 24, 2011, at 6:05 PM, Jarod Wilson wrote:

> On Thu, Mar 24, 2011 at 11:31:08PM +0300, Dan Carpenter wrote:
>> On Thu, Mar 24, 2011 at 04:07:00PM -0400, Jarod Wilson wrote:
>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> 
>> Jarod, there is a lot of information missing from your change log...  :/
> 
> Hrm, I'm building the media stack with all warnings fatal, so this was
> just a quick fix to silence the compiler warning, didn't really look into
> it at all.
> 
>>> CC: devel@driverdev.osuosl.org
>>> ---
>>> drivers/staging/tm6000/tm6000-video.c |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
>>> index c80a316..bfebedd 100644
>>> --- a/drivers/staging/tm6000/tm6000-video.c
>>> +++ b/drivers/staging/tm6000/tm6000-video.c
>>> @@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long len,
>>> 	unsigned long header = 0;
>>> 	int rc = 0;
>>> 	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
>>> -	struct tm6000_buffer *vbuf;
>>> +	struct tm6000_buffer *vbuf = NULL;
>>> 	char *voutp = NULL;
>>> 	unsigned int linewidth;
>>> 
>> 
>> This looks like a real bug versus just a GCC warning.  It was introduced
>> in 8aff8ba95155df "[media] tm6000: add radio support to the driver".
>> I've added Dmitri to the CC list.
> 
> Thanks much, will try to pay more attention next time. ;)

So I was just circling back around on this one, and took some time to read
the actual code and the radio support addition. After doing so, I don't
see why the patch I proposed wouldn't do. The buffer is only manipulated
if !dev->radio or if vbuf is non-NULL (the memcpy call). If its initialized
to NULL, it only gets used exactly as it did before 8aff8ba9 when
!dev->radio, and if its not been used or its NULL following manipulations
protected by !dev->radio, it doesn't get copied. What is the "real bug" I
am missing there? (Or did I already miss a patch posted to linux-media
addressing it?)

Thanks much,

-- 
Jarod Wilson
jarod@wilsonet.com

[PATCH v2] tm6000: fix vbuf may be used uninitialized

[Jarod Wilson]

In commit 8aff8ba95155df, most of the manipulations to vbuf inside
copy_streams were gated on if !dev->radio, but one place that touches
vbuf lays outside those gates -- a memcpy of vbuf isn't NULL. If we
initialize vbuf to NULL, that memcpy will never happen in the case where
we do have dev->radio, and otherwise, in the !dev->radio case, the code
behaves exactly like it did prior to 8aff8ba95155df.

While we're at it, also fix an incorrectly indented closing brace for
one of the sections touching vbuf that is conditional on !dev->radio.

v2: add a detailed commit log and fix that brace

CC: Dan Carpenter <error27@gmail.com>
CC: Dmitri Belimov <d.belimov@gmail.com>
CC: devel@driverdev.osuosl.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/staging/tm6000/tm6000-video.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
index c80a316..8b971a0 100644
--- a/drivers/staging/tm6000/tm6000-video.c
+++ b/drivers/staging/tm6000/tm6000-video.c
@@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long len,
 	unsigned long header = 0;
 	int rc = 0;
 	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
-	struct tm6000_buffer *vbuf;
+	struct tm6000_buffer *vbuf = NULL;
 	char *voutp = NULL;
 	unsigned int linewidth;
 
@@ -318,7 +318,7 @@ static int copy_streams(u8 *data, unsigned long len,
 					if (pos + size > vbuf->vb.size)
 						cmd = TM6000_URB_MSG_ERR;
 					dev->isoc_ctl.vfield = field;
-			}
+				}
 				break;
 			case TM6000_URB_MSG_VBI:
 				break;
-- 
1.7.1

Re: [PATCH v2] tm6000: fix vbuf may be used uninitialized

[Dmitri Belimov]

Hi

I think it's good.
No regression, all works well.

With my best regards, Dmitry.

> In commit 8aff8ba95155df, most of the manipulations to vbuf inside
> copy_streams were gated on if !dev->radio, but one place that touches
> vbuf lays outside those gates -- a memcpy of vbuf isn't NULL. If we
> initialize vbuf to NULL, that memcpy will never happen in the case
> where we do have dev->radio, and otherwise, in the !dev->radio case,
> the code behaves exactly like it did prior to 8aff8ba95155df.
> 
> While we're at it, also fix an incorrectly indented closing brace for
> one of the sections touching vbuf that is conditional on !dev->radio.
> 
> v2: add a detailed commit log and fix that brace
> 
> CC: Dan Carpenter <error27@gmail.com>
> CC: Dmitri Belimov <d.belimov@gmail.com>
> CC: devel@driverdev.osuosl.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/staging/tm6000/tm6000-video.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/tm6000/tm6000-video.c
> b/drivers/staging/tm6000/tm6000-video.c index c80a316..8b971a0 100644
> --- a/drivers/staging/tm6000/tm6000-video.c
> +++ b/drivers/staging/tm6000/tm6000-video.c
> @@ -228,7 +228,7 @@ static int copy_streams(u8 *data, unsigned long
> len, unsigned long header = 0;
>  	int rc = 0;
>  	unsigned int cmd, cpysize, pktsize, size, field, block,
> line, pos = 0;
> -	struct tm6000_buffer *vbuf;
> +	struct tm6000_buffer *vbuf = NULL;
>  	char *voutp = NULL;
>  	unsigned int linewidth;
>  
> @@ -318,7 +318,7 @@ static int copy_streams(u8 *data, unsigned long
> len, if (pos + size > vbuf->vb.size)
>  						cmd =
> TM6000_URB_MSG_ERR; dev->isoc_ctl.vfield = field;
> -			}
> +				}
>  				break;
>  			case TM6000_URB_MSG_VBI:
>  				break;
> -- 
> 1.7.1
>