I checked the code with cppcheck (version from git). Also I created gtk config for cppcheck - if you are interested in, you could help me to fill it to improve analysis: https://gist.github.com/scriptum/7282198
cppcheck cannot check GTK programs because glib's design is not heap-friendly: most functions needs freeing memory and cppcheck doesn't know about them. In new versions you can specify glib functions in config (there is example one), but this is huge work...
Geany: ./cppcheck ../geany --library=gtk `pkg-config --cflags glib-2.0` \ --max-configs=1 -j32 -q --template=gcc [../geany/src/editor.c:4866]: (error) Memory leak: f
This is the one and real (return-after-malloc) bug. g_return_if_fail is danger: there should be g_goto_end_if_fail and nothing other to make sure you finished transaction. You will find similar leak in plugins. This is _very_ dangerous because it doesn't look like a macro and hides return keyword from you inside!
Plugins are more interesting: https://gist.github.com/scriptum/7282262
Treebrowser bugs are false-pos. But they are appeared due to bad pattern (variable reuse):
treebrowser_browse(gchar *directory, gpointer parent) ... directory = g_strconcat(directory, G_DIR_SEPARATOR_S, NULL); ... g_free(directory);
Next leak markdown/src/conf.c:457 g_key_file_to_data never returns error (regarding to doc), this is false-pos but useless code could be removed
Next leak ../geany-plugins/scope/src/menu.c:461 flase-pos, but it's a bad pattern using pointer after free.
All others aren't false-pos and should be fixed.
-- Best regards, Pavel Roschin aka RPG
Hi Pavel,
First, thanks a lot for looking at better checking the code, that's a great thing :)
Le 02/11/2013 20:41, Pavel Roschin a écrit :
I checked the code with cppcheck (version from git). Also I created gtk config for cppcheck - if you are interested in, you could help me to fill it to improve analysis: https://gist.github.com/scriptum/7282198
cppcheck cannot check GTK programs because glib's design is not heap-friendly: most functions needs freeing memory and cppcheck doesn't know about them. In new versions you can specify glib functions in config (there is example one), but this is huge work...
Why did it somewhat work in the past? Was it luck and poor checking or did it know about some of them? But indeed, it couldn't really know without a huge config file like yours.
Geany: ./cppcheck ../geany --library=gtk `pkg-config --cflags glib-2.0` \ --max-configs=1 -j32 -q --template=gcc [../geany/src/editor.c:4866]: (error) Memory leak: f
Fixed, thanks.
This is the one and real (return-after-malloc) bug. g_return_if_fail is danger: there should be g_goto_end_if_fail and nothing other to make sure you finished transaction. You will find similar leak in plugins. This is _very_ dangerous because it doesn't look like a macro and hides return keyword from you inside!
Well, yeah, it could be misleading in existing code, but when you add it you know it will return; and it's quite nice when you're use to it. But that's right that it needs to be cared about.
Also, remember that g_return*_if_fail() is supposed to check for an *bug* in the caller code, so leaking in this case isn't IMO as much of a big deal as if it was in normal code path.
Regards, Colomban
Why did it somewhat work in the past? Was it luck and poor checking or did it know about some of them? But indeed, it couldn't really know without a huge config file like yours.
Yes, cppcheck just check very clear bugs. It cannot check leaking through pointer sharing and so on, it doesn't know that util_* functions return allocated memory and just skip them to do less false-positives. But with config detailed enough it will be powerful tool.
Well, yeah, it could be misleading in existing code, but when you add it you know it will return; and it's quite nice when you're use to it. But that's right that it needs to be cared about.
Also, remember that g_return*_if_fail() is supposed to check for an *bug* in the caller code, so leaking in this case isn't IMO as much of a big deal as if it was in normal code path.
Actually such kind of bugs appear not only when you add the code but when you modify it. When you create a new function you carefully put all things at right place but next time you don't know who and how will modify this piece of code and this may cause a bug just because second look at this code it less "careful".
Just take a look and you will see that I was right: https://github.com/geany/geany/commit/8a817e694bafdf5106c672133902b5daef0fe8...
This is not "add new code bug", this is "modify code without checking around" bug. And checking g_*if_fail is really hard because you check return statements, you don't see them and committer just skipped this g_return_if_fail. Yes, defines are very evil:)
Maybe this is a bad example because there was a definitely-leak before this commit that transformed to possible-leak.
-- Best regards, Pavel Roschin aka RPG
Updated new config:
https://gist.github.com/scriptum/7282198
new few leaks (g_return*, again!):
../geany/src/utils.c:868: error: Memory leak: buffer ../geany/src/utils.c:1811: error: Memory leak: str
../geany/tagmanager/ctags/sort.c:215: error: Mismatching allocation and deallocation: table Here should be g_free, not just free.
Hi Pavel,
First, thanks a lot for looking at better checking the code, that's a great thing :)
Le 02/11/2013 20:41, Pavel Roschin a écrit :
I checked the code with cppcheck (version from git). Also I created gtk config for cppcheck - if you are interested in, you could help me to fill it to improve analysis: https://gist.github.com/scriptum/7282198
cppcheck cannot check GTK programs because glib's design is not heap-friendly: most functions needs freeing memory and cppcheck doesn't know about them. In new versions you can specify glib functions in config (there is example one), but this is huge work...
Why did it somewhat work in the past? Was it luck and poor checking or did it know about some of them? But indeed, it couldn't really know without a huge config file like yours.
Geany: ./cppcheck ../geany --library=gtk `pkg-config --cflags glib-2.0` \ --max-configs=1 -j32 -q --template=gcc [../geany/src/editor.c:4866]: (error) Memory leak: f
Fixed, thanks.
This is the one and real (return-after-malloc) bug. g_return_if_fail is danger: there should be g_goto_end_if_fail and nothing other to make sure you finished transaction. You will find similar leak in plugins. This is _very_ dangerous because it doesn't look like a macro and hides return keyword from you inside!
Well, yeah, it could be misleading in existing code, but when you add it you know it will return; and it's quite nice when you're use to it. But that's right that it needs to be cared about.
Also, remember that g_return*_if_fail() is supposed to check for an *bug* in the caller code, so leaking in this case isn't IMO as much of a big deal as if it was in normal code path.
Regards, Colomban _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
-- Best regards, Pavel Roschin aka RPG
Le 03/11/2013 01:19, Pavel Roschin a écrit :
Updated new config:
https://gist.github.com/scriptum/7282198
new few leaks (g_return*, again!):
../geany/src/utils.c:868: error: Memory leak: buffer ../geany/src/utils.c:1811: error: Memory leak: str
../geany/tagmanager/ctags/sort.c:215: error: Mismatching allocation and deallocation: table Here should be g_free, not just free.
All three fixed, and the elements of `table` also were deallocated using the wrong function.
Thanks, Colomban
On Sat, 2 Nov 2013 23:41:08 +0400 Pavel Roschin roshin@scriptumplus.ru wrote:
I checked the code with cppcheck (version from git). Also I created gtk config for cppcheck - if you are interested in, you could help me to fill it to improve analysis: https://gist.github.com/scriptum/7282198
Next leak ../geany-plugins/scope/src/menu.c:461 flase-pos, but it's a bad pattern using pointer after free.
Checking if it pointed to anything other than NULL is pretty normal IMHO. Adding an extra temporary variable just to keep cppcheck happy is simple, but I don't see enough merit, sorry.