[Bug 72088] New: cppcheck report

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

Priority: medium
Bug ID: 72088
Assignee: openchrome-***@lists.freedesktop.org
Summary: cppcheck report
Severity: normal
Classification: Unclassified
OS: All
Reporter: ***@yahoo.fr
Hardware: Other
Status: NEW
Version: git
Component: Driver/openchrome
Product: xorg

Created attachment 89921
--> https://bugs.freedesktop.org/attachment.cgi?id=89921&action=edit
cppcheck report

I runned cppcheck git updated today on OpenChrome git updated today.

There are certainly false positives since cppcheck is a static analyzer for C++
(I think about "scope of <var> can be reduced) but for the rest, it could help.
eg:
[src/via_bandwidth.c:485] -> [src/via_bandwidth.c:483]: (style) Found duplicate
branches for 'if' and 'else'.
[src/via_bandwidth.c:502] -> [src/via_bandwidth.c:500]: (style) Found duplicate
branches for 'if' and 'else'.

[src/via_driver.c:1107]: (error) Memory pointed to by 'pEnt' is freed twice.
[src/via_dri.c:481]: (error) Memory leak: pVIAConfigPtrs

[src/via_driver.c:901]: (error) Uninitialized variable: old_width
[src/via_driver.c:900]: (error) Uninitialized variable: old_height
[src/via_driver.c:902]: (error) Uninitialized variable: old_dwidth

I attached the full report (I used --enable=all for running cppcheck).

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #1 from Mario Rugiero <***@gmail.com> ---
Created attachment 90136
--> https://bugs.freedesktop.org/attachment.cgi?id=90136&action=edit
This patch should fix all or most of the memory related warnings.

Should fix the following warnings:

[src/via_3d.c:260] -> [src/via_3d.c:261]: (warning) Possible null pointer
dereference: v3d - otherwise it is redundant to check it against null.
[src/via_dri.c:481]: (error) Memory leak: pVIAConfigPtrs
[src/via_driver.c:1107]: (error) Memory pointed to by 'pEnt' is freed twice.
[src/via_exa.c:222] -> [src/via_exa.c:220]: (warning) Possible null pointer
dereference: cb - otherwise it is redundant to check it against null.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #2 from Mario Rugiero <***@gmail.com> ---
Created attachment 90137
--> https://bugs.freedesktop.org/attachment.cgi?id=90137&action=edit
Simpler fix for the via_exa.c null dereference.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #3 from Mario Rugiero <***@gmail.com> ---
Just ran cppcheck again.

I will explain the errors I'll fix here, and then provide the patch.

First, this one:

[src/via_driver.c:1109]: (error) Memory pointed to by 'pEnt' is freed twice.

This error is caused because the memory is always freed in line 1031, and all
subsequent calls to this are for freeing in the case of errors. This calls will
be plainly erased, as not only they aren't checked against it having been freed
before (it is, independently of the path, as it is checked only against null
and in the first level of indentation, so I assume it is not inside another
check), but actually it is already freed always, and not needed anymore.

Then, this:
[src/via_exa.c:799] -> [src/via_exa.c:808]: (performance) Variable
'nPOTSupported' is reassigned a value before the old one has been used.

This one is pretty much trivial, and the fix could be safely ommited. I provide
it, anyway, so someone else makes the choice.

There are also, pretty thorough to see, fixes in the registers tool, related
all to formatting on printf.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #4 from Mario Rugiero <***@gmail.com> ---
Created attachment 90809
--> https://bugs.freedesktop.org/attachment.cgi?id=90809&action=edit
Fixes double frees, see my previous comment for a complete explanation.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #5 from Mario Rugiero <***@gmail.com> ---
Created attachment 90810
--> https://bugs.freedesktop.org/attachment.cgi?id=90810&action=edit
(Trivial) Patch for a reassignment without using the previous value.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #6 from Mario Rugiero <***@gmail.com> ---
Created attachment 90811
--> https://bugs.freedesktop.org/attachment.cgi?id=90811&action=edit
Formatting fixes for registers.c

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #7 from Mario Rugiero <***@gmail.com> ---
Created attachment 90813
--> https://bugs.freedesktop.org/attachment.cgi?id=90813&action=edit
A (possible?) fix for the duplicate branches error.

Please, review that this actually is what was intended, as I'm just basing in
the pattern I observed in the other cases (it seems the else is supposed to
have doubled the value compared to the other branch, but I'm not sure).

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #8 from Mario Rugiero <***@gmail.com> ---
Forgot to mark as patches the patches for the duplicate branches and for the
reassignment. If anyone is able to mark them (to ease review), I'll be
thankful. Otherwise, I'll add the patches again with the correct tag.

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

Mario Rugiero <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #90810|0 |1
is patch| |

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

Mario Rugiero <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #90813|0 |1
is patch| |

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

Alan Coopersmith <***@oracle.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
QA Contact| |xorg-***@lists.x.org

[b***@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=72088

--- Comment #9 from Julien Nabet <***@yahoo.fr> ---
Seeing git history, git log -1 gives:
commit 46a4ef37a1fc793ea85950f9579783e9af3a6b36
Author: Xavier Bachelot <***@bachelot.org>
Date: Fri Oct 18 18:04:50 2013 +0200

Add pciid for Lenovo M3120C

Is it abandonned or did the repository move?