Re: Weekly cppcheck run against Aug 27 Git Tree

[chris ahrendt]

Mike Kaplinskiy wrote:
> On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt wrote:
> > This is the result of running cppcheck 1.35 with the --all parm against
> > the august 27th Git tree:
> >
> > [../wine-git/dlls/dbghelp/msc.c:88]: (possible error) Array index out of
> > bounds
> > [../wine-git/dlls/dbghelp/msc.c:89]: (possible error) Array index out of
> > bounds
>
> False positive, apparently the numbers are hardcoded as:
> 72     char        msg[128];
> 88         msg[10 + 3 * 16] = ' '; // = 58<127
> 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
> > [../wine-git/dlls/shell32/cpanelfolder.c:562]: (error) Possible null
> > pointer dereference: rgfInOut
> > [../wine-git/dlls/shell32/shfldr_desktop.c:437]: (error) Possible null
> > pointer dereference: rgfInOut
> > [../wine-git/dlls/shell32/shfldr_fs.c:577]: (error) Possible null
> > pointer dereference: rgfInOut
> > [../wine-git/dlls/shell32/shfldr_mycomp.c:474]: (error) Possible null
> > pointer dereference: rgfInOut
> > [../wine-git/dlls/shell32/shfldr_netplaces.c:352]: (error) Possible null
> > pointer dereference: rgfInOut
>
> It doesn't like the ternary operator? These lines are TRACE lines with
> one of the args being ``rgfInOut ? *rgfInOut : 0''. False positive.
> > [../wine-git/dlls/user32/tests/msg.c:63]: (error) Invalid number of
> > character ({). Can't process file.
> > [../wine-git/dlls/winealsa.drv/waveinit.c:745]: (possible error) Buffer
> > overrun
>
> 739             char defaultpcmname[256];
> 745             sprintf(defaultpcmname, "default");
>
> Something is wrong with cppcheck... False positive.
> > [../wine-git/dlls/wined3d/arb_program_shader.c:907]: (possible error)
> > Buffer overrun
> > [../wine-git/dlls/wined3d/arb_program_shader.c:915]: (possible error)
> > Buffer overrun
> > [../wine-git/dlls/wined3d/glsl_shader.c:3416]: (possible error) Buffer
> > overrun
> > [../wine-git/dlls/wined3d/glsl_shader.c:3418]: (possible error) Buffer
> > overrun
> > [../wine-git/dlls/wined3d/glsl_shader.c:3519]: (possible error) Buffer
> > overrun
> > [../wine-git/dlls/wined3d/glsl_shader.c:3521]: (possible error) Buffer
> > overrun
>
> Not checking those - i don't even pretend to understand how modern
> graphics works.
> > [../wine-git/dlls/wineoss.drv/mixer.c:1458]: (possible error) Buffer overrun
>
> So...it picks
> 1455         char name[32];
> 1458             sprintf(name, "/dev/mixer");
>
> as an error, but not:
>
> 1460             sprintf(name, "/dev/mixer%d", i);
>
> False positive.
> > [../wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer
> > dereference: dmW
>
> This one is actually a bug, the null check is below this line. All the
> callers check for nulls, though.
> > [../wine-git/programs/oleview/pane.c:152]: (error) Possible null pointer
> > dereference: hWndCreated
>
> Also a bug, and a very real one. Coincidentally, the null check on the
> next line is also wrong (should be if (!*hWndCreated) )
> > [../wine-git/programs/winetest/main.c:114]: (possible error) Buffer overrun
> > [../wine-git/programs/winetest/main.c:116]: (possible error) Buffer overrun
> > [../wine-git/programs/winetest/main.c:119]: (possible error) Buffer overrun
> > [../wine-git/programs/winetest/main.c:121]: (possible error) Buffer overrun
>
> More of sprintf with just a string nonsense. False positive.
> > [../wine-git/server/file.c:235]: (possible error) Buffer overrun
>
> Also sprintf nonsense, but slightly more dangerous. The buffer is
> declared with [16] and the string is of length 14+1, so a few more
> bytes wouldn't hurt. :)
> > Chris
>
> If someone could send patches for the few bugs that would be nice.
>
> Chris - cppcheck is clearly crazy about sprintf's and ternary
> operators. You might want to report that.
>
> Mike.

Mike While yes the hard coded one above is a false positive... I would 
argue its still a bug that probably needs to get fixed...

chris

Weekly cppcheck run against Aug 27 Git Tree

[Ben Klein]

2009/8/28 chris ahrendt :
> Mike Kaplinskiy wrote:
> > On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt wrote:
> > > This is the result of running cppcheck 1.35 with the --all parm against
> > > the august 27th Git tree:
> > >
> > > [../wine-git/dlls/dbghelp/msc.c:88]: (possible error) Array index out of
> > > bounds
> > > [../wine-git/dlls/dbghelp/msc.c:89]: (possible error) Array index out of
> > > bounds
> >
> > False positive, apparently the numbers are hardcoded as:
> > 72     char        msg[128];
> > 88         msg[10 + 3 * 16] = ' '; // = 58<127
> > 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
>
> Mike While yes the hard coded one above is a false positive... I would
> argue its still a bug that probably needs to get fixed...

I don't follow this logic. How is it a bug (in Wine) exactly?

Weekly cppcheck run against Aug 27 Git Tree

[chris ahrendt]

----- Original Message ----
From: Ben Klein 
To: chris ahrendt 
Cc: [e-mail]
Sent: Thursday, August 27, 2009 10:06:56 PM
Subject: Re: Weekly cppcheck run against Aug 27 Git Tree

2009/8/28 chris ahrendt :
> Mike Kaplinskiy wrote:
> > On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt wrote:
> > > This is the result of running cppcheck 1.35 with the --all parm against
> > > the august 27th Git tree:
> > >
> > > [../wine-git/dlls/dbghelp/msc.c:88]: (possible error) Array index out of
> > > bounds
> > > [../wine-git/dlls/dbghelp/msc.c:89]: (possible error) Array index out of
> > > bounds
> >
> > False positive, apparently the numbers are hardcoded as:
> > 72     char        msg[128];
> > 88         msg[10 + 3 * 16] = ' '; // = 58<127
> > 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
>
> Mike While yes the hard coded one above is a false positive... I would
> argue its still a bug that probably needs to get fixed...
> I don't follow this logic. How is it a bug (in Wine) exactly?

I thought one of the programming standards was the fact you don't hard code values IE 10+3*16... 
it should probably be :

msg_blank = 10+3*16; // These go into header files
msg_length = 128; // This goes into header file

char msg[msg_length];
memset(msg, 0, sizeof(msg));
memset(msg, ' ', msg_blank); // or it could even be msg[msg_blank] = ' '; if only position 58 needs to be a ' ' , but I prefer the first method.

does pretty much the same thing except for one point.
Whatever is in the local stack at the point of assigning the msg buffer will be still there unless you initialise it to null.

chris

Weekly cppcheck run against Aug 27 Git Tree

[James McKenzie]

chris ahrendt wrote:
> ----- Original Message ----
> From: Ben Klein 
> To: chris ahrendt 
> Cc: [e-mail]
> Sent: Thursday, August 27, 2009 10:06:56 PM
> Subject: Re: Weekly cppcheck run against Aug 27 Git Tree
>
> 2009/8/28 chris ahrendt :
> > Mike Kaplinskiy wrote:
> > > On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt wrote:
> > > > This is the result of running cppcheck 1.35 with the --all parm against
> > > > the august 27th Git tree:
> > > >
> > > > [../wine-git/dlls/dbghelp/msc.c:88]: (possible error) Array index out of
> > > > bounds
> > > > [../wine-git/dlls/dbghelp/msc.c:89]: (possible error) Array index out of
> > > > bounds
> > >
> > > False positive, apparently the numbers are hardcoded as:
> > > 72     char        msg[128];
> > > 88         msg[10 + 3 * 16] = ' '; // = 58<127
> > > 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
> >
> > Mike While yes the hard coded one above is a false positive... I would
> > argue its still a bug that probably needs to get fixed...
> > I don't follow this logic. How is it a bug (in Wine) exactly?
>
> I thought one of the programming standards was the fact you don't hard code values IE 10+3*16... 
> it should probably be :
>
> msg_blank = 10+3*16; // These go into header files
> msg_length = 128; // This goes into header file
>
> char msg[msg_length];
> memset(msg, 0, sizeof(msg));
> memset(msg, ' ', msg_blank); // or it could even be msg[msg_blank] = ' '; if only position 58 needs to be a ' ' , but I prefer the first method.
>
> does pretty much the same thing except for one point.
> Whatever is in the local stack at the point of assigning the msg buffer will be still there unless you initialise it to null.

I like your logic, but why not just do this:

msg_blank= 58;
msg_yada_yada=75;
msg_length=128;

This sets all of the variables using plain values and then they can be
updated as necessary.

James McKenzie

Weekly cppcheck run against Aug 27 Git Tree

[chris ahrendt]

Ok CPPCheck guys have repired the false positive but now get this:

$ ./cppcheck -q -a ../wine/wine/dlls/wineoss.drv/mixer.c 
../wine/wine/dlls/wineoss.drv/mixer.c:576: (error) Resource leak: mixer
../wine/wine/dlls/wineoss.drv/mixer.c:600: (error) Resource leak: mixer
../wine/wine/dlls/wineoss.drv/mixer.c:1454: (error) Resource leak: mixer

can someone tell me if these are false positives as well?

chris

Weekly cppcheck run against Aug 27 Git Tree

[Marcus Meissner]

On Fri, Aug 28, 2009 at 02:16:40PM -0700, chris ahrendt wrote:
> Ok CPPCheck guys have repired the false positive but now get this:
>
> $ ./cppcheck -q -a ../wine/wine/dlls/wineoss.drv/mixer.c 
> ../wine/wine/dlls/wineoss.drv/mixer.c:576: (error) Resource leak: mixer
> ../wine/wine/dlls/wineoss.drv/mixer.c:600: (error) Resource leak: mixer
> ../wine/wine/dlls/wineoss.drv/mixer.c:1454: (error) Resource leak: mixer
>
> can someone tell me if these are false positives as well?

if ((mixer = open(mix->dev_name, O_RDWR)) >= 0) {
        if (ioctl(mixer, SOUND_MIXER_READ_RECSRC, &mask) >= 0) {
            ret = TRUE;
        } else {
            ERR("ioctl(%s, SOUND_MIXER_READ_RECSRC) failed (%s)\n",
                mix->dev_name, strerror(errno));
        }
        close(mixer);
    }

The code looks good to me.

Ciao, Marcus

Weekly cppcheck run against Aug 27 Git Tree

[Mike Kaplinskiy]

On Fri, Aug 28, 2009 at 5:22 PM, Marcus Meissner wrote:
> On Fri, Aug 28, 2009 at 02:16:40PM -0700, chris ahrendt wrote:
> > Ok CPPCheck guys have repired the false positive but now get this:
> >
> > $ ./cppcheck -q -a ../wine/wine/dlls/wineoss.drv/mixer.c
> > ../wine/wine/dlls/wineoss.drv/mixer.c:576: (error) Resource leak: mixer
> > ../wine/wine/dlls/wineoss.drv/mixer.c:600: (error) Resource leak: mixer
> > ../wine/wine/dlls/wineoss.drv/mixer.c:1454: (error) Resource leak: mixer
> >
> > can someone tell me if these are false positives as well?
>
> if ((mixer = open(mix->dev_name, O_RDWR)) >= 0) {
>        if (ioctl(mixer, SOUND_MIXER_READ_RECSRC, &mask) >= 0) {
>            ret = TRUE;
>        } else {
>            ERR("ioctl(%s, SOUND_MIXER_READ_RECSRC) failed (%s)\n",
>                mix->dev_name, strerror(errno));
>        }
>        close(mixer);
>    }
>
> The code looks good to me.
>
> Ciao, Marcus

Yep this looks fine. For experimentation sake, see if cppcheck prefers
!= -1 as opposed to >= 0. Also a bug that they should probably know
about.

Mike.