On 25/03/2012 19:22, Matthew Brush wrote:

Weird, it builds OK here with `--enable-cppcheck` and `make check`. It has a few warnings about the compiled C code but otherwise seems fine. Can you tell what the error/failure message is?

Cheers, Matthew Brush

The debugger one: make[4]: Entering directory `/home/sardemff7/Developpement/Geany/geany-plugins/debugger/src' /usr/bin/cppcheck -q --template gcc --error-exitcode=2 . dbm_gdb.c:1483: error: Return value of allocation function g_strdup_printf is not used.

The multiterm one: make[3]: Entering directory `/home/sardemff7/Developpement/Geany/geany-plugins/multiterm/src' /usr/bin/cppcheck -q --template gcc --error-exitcode=2 . cppcheck: error: could not find or open any of the paths given.

I’m using the latest cppcheck commit.

On 25/03/2012 19:40, Matthew Brush wrote:

It almost sounds like it can't see the C files compiled by Valac. Is there C files in `multiterm/src/`?

Nothing. This plugin is disabled. If you think it should run cppcheck on Vala generated code (I’m not sure it’s worth it), at least it shouldn’t when it’s disabled.

For the debugger one, I’d guess it’s a new feature added in cppcheck and not released yet.

Le 25/03/2012 17:46, Quentin Glidic a écrit :

Hello,

While running geany-plugins tests, I hit two failures.

The first one is that cppcheck is not happy about Vala, and since multiterm is fully in Vala, it fails.

ACK, running cppcheck on a non-C plugin seems stupid anyway.

The second one is that it couldn’t detect an assignment nested in an array.

ACK, "initializer element is not computable at load time" which is bad C anyway.

Attached two simple patches to fix these.

Cheers

BTW, my cppcheck (1.53) finds 5 things in debugger:

dbm_gdb.c:1304: error: Possible null pointer dereference: record dbm_gdb.c:1579: error: Possible null pointer dereference: record dbm_gdb.c:1700: error: Possible null pointer dereference: record dbm_gdb.c:967: error: Memory pointed to by 'record' is freed twice. debug.c:502: error: Possible null pointer dereference: expression

The first looks like a real possible problem, if exec_sync_command() returns RC_DONE at line 618 because wait4prompt is false.

The others *may* be false positive because cppcheck guesses that if you initialize a variable to NULL you expect that passing it "by reference" may not change its value. (e.g. foo=NULL; fun(&foo); ...)

However they seem to be real problems to me here. It seems to me that exec_sync_command() can return RC_DONE either if it has or not set the command_record argument, in which case the caller can't know whether it has to free the value or if it wasn't set. So, the caller must pass a properly NULL-initialized variable; but then the caller also have to check whether that variable is non-NULL before dereferencing it -- and must free() it whatever happens.

So, IIUC:

in dbm_gdb.c on lines 1304, 1579, 1700: * record should be checked against NULL before passing it to strstr()

in dbm_gdb.c on line 1579: * record should be freed even when rc != RC_DONE (line 1578)

in dbm_gdb.c on line 967: * record should be re-initialized to NULL after the first free on line 963

in debug.c on line 502: * IIRC expression might be NULL if it happens that the W_EXPRESSION column isn't set in the model, so a NULL check should be done before passing expression to strlen() * using strlen() to check whether a string is empty seems sub-optimal too, and you could consider using the NZV() macro from geany's utils.h -- and this macro handles NULL too

That's all for now :) There are also some stuff that should be fixed for good C practices and/or C89 compliance, but I'll check these later.

Cheers, Colomban