tm6000: fix vbuf may be used uninitialized

Message ID 1300997220-4354-1-git-send-email-jarod@redhat.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Jarod Wilson March 24, 2011, 8:07 p.m. UTC
  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(-)
  

Comments

Dan Carpenter March 24, 2011, 8:31 p.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Jarod Wilson March 24, 2011, 10:05 p.m. UTC | #2
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 April 11, 2011, 9:48 p.m. UTC | #3
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,
  

Patch

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;