Created attachment 89921 [details] 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).
Created attachment 90136 [details] [review] 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.
Created attachment 90137 [details] [review] Simpler fix for the via_exa.c null dereference.
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.
Created attachment 90809 [details] [review] Fixes double frees, see my previous comment for a complete explanation.
Created attachment 90810 [details] [review] (Trivial) Patch for a reassignment without using the previous value.
Created attachment 90811 [details] [review] Formatting fixes for registers.c
Created attachment 90813 [details] [review] 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).
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.
Seeing git history, git log -1 gives: commit 46a4ef37a1fc793ea85950f9579783e9af3a6b36 Author: Xavier Bachelot <xavier@bachelot.org> Date: Fri Oct 18 18:04:50 2013 +0200 Add pciid for Lenovo M3120C Is it abandonned or did the repository move?
Julien, thanks for the report. Mario, thanks for the patches. All fixed in git master. Regards, Xavier
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.