Weekly cppcheck run against Aug 27 Git Tree

[chris ahrendt]

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
[../wine-git/dlls/msvcrt/tests/file.c:982]: (error) Deallocating a 
deallocated pointer: stream2
[../wine-git/dlls/msvcrt/tests/file.c:966]: (error) Resource leak: stream3
[../wine-git/dlls/msvcrt/tests/file.c:973]: (error) Resource leak: stream4
[../wine-git/dlls/msvcrt/tests/heap.c:433]: (possible error) Memory 
leak: mem
[../wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd
[../wine-git/dlls/rpcrt4/tests/server.c:1189]: (possible error) Array 
index out of bounds
[../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
[../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
[../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
[../wine-git/dlls/wineoss.drv/mixer.c:1458]: (possible error) Buffer overrun
[../wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer 
dereference: dmW
[../wine-git/programs/oleview/pane.c:152]: (error) Possible null pointer 
dereference: 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
[../wine-git/server/file.c:235]: (possible error) Buffer overrun
[../wine-git/tools/fnt2bdf.c:656]: (error) Resource leak: fd
[../wine-git/tools/fnt2fon.c:303]: (error) Memory leak: file_lens
[../wine-git/tools/makedep.c:337]: (error) Resource leak: file
[../wine-git/tools/widl/write_msft.c:2540]: (error) Deallocating a 
deallocated pointer: fd
[../wine-git/tools/winedump/pe.c:1549]: (possible error) Memory leak: map


Chris

[Mike Kaplinskiy]

On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt<celticht32(a)yahoo.com> 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.

[James McKenzie]

Mike Kaplinskiy wrote:
> On Thu, Aug 27, 2009 at 3:52 PM, chris ahrendt<celticht32(a)yahoo.com> 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

Why not just say 58 instead of the complex math?
> 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127

Same here.   Why not 75 instead of the math formula? 

This is relying on a certain path for math which may or may not be true
(RPN anyone?)  It was best practice to use constants vice complex math
to determine array locations or to set and test a variable before the call.

I could see this if a variable was used in the math formula that was
checked for out of bounds, but this is not the case.

James McKenzie

[Dmitry Timoshkov]

"James McKenzie" <jjmckenzie51(a)earthlink.net> wrote:
> > False positive, apparently the numbers are hardcoded as:
> > 72     char        msg[128];
> > 88         msg[10 + 3 * 16] = ' '; // = 58<127
>
> Why not just say 58 instead of the complex math?
> > 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
>
> Same here.   Why not 75 instead of the math formula?

Because that's done that way to make it easier to understand
the logic for the programmer. If a tool can't cope with it -
that tool is broken.

-- 
Dmitry.

[Ben Klein]

2009/8/28 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
> "James McKenzie" <jjmckenzie51(a)earthlink.net> wrote:
> > > False positive, apparently the numbers are hardcoded as:
> > > 72     char        msg[128];
> > > 88         msg[10 + 3 * 16] = ' '; // = 58<127
> >
> > Why not just say 58 instead of the complex math?
> > > 89         msg[10 + 3 * 16 + 1 + 16] = '\0'; // = 75<127
> >
> > Same here.   Why not 75 instead of the math formula?
>
> Because that's done that way to make it easier to understand
> the logic for the programmer. If a tool can't cope with it -
> that tool is broken.

I just took a look at the code, and I agree with Dmitry. In context,
these lines appear immediately after a loop structure that is
populating the array. The "10 + 3 * 16" calculated constants make
perfect sense in the context of the loop (note that there is also a "+
0" on line 82). Changing these to single constant values would make it
much harder for the casual observer to read.

Note that the compiler will optimise this to a single constant anyway.

Either way, it's not an actual *bug* unless the values are wrong.

[Austin English]

On Fri, Aug 28, 2009 at 4:06 AM, Ben Klein<shacklein(a)gmail.com> wrote:
> Changing these to single constant values would make it
> much harder for the casual observer to read.

That's what comments are for.

-- 
-Austin

[Juan Lang]

> > Changing these to single constant values would make it
> > much harder for the casual observer to read.
>
> That's what comments are for.

Actually, comments are for code that's not understandable without
them.  The code is more readable as it is, IMHO, than it would be by
replacing the expression--perfectly natural in context--with its
evaluation, or with a symbolic constant.
--Juan

[James McKenzie]

Juan Lang wrote:
> > > Changing these to single constant values would make it
> > > much harder for the casual observer to read.
> >
> > That's what comments are for.
>
> Actually, comments are for code that's not understandable without
> them.  The code is more readable as it is, IMHO, than it would be by
> replacing the expression--perfectly natural in context--with its
> evaluation, or with a symbolic constant.

Actually, I'm with Austin on this one.  A comment on the line that the
code was in would be most helpful and would explain the use of the
complex math formulae rather than having to guess at what is going on. 
That is why comments exist.  To prevent guessing why certain code is why
is the way it was written and for others (remember you can get run over
by a bus, get hit by a car, get Swine Flu and be out for weeks) to not
have to guess.  I believe in overcommenting rather than under.  BTW,
that is what I was taught and is ITIL BBP.

James McKenzie