View previous topic :: View next topic |
Author |
Message |
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Thu Feb 05, 2015 6:57 pm Post subject: Checking miniupnpc code |
|
|
I applied cppcheck (free static code analizer) to a project, where miniupnpc is used as a library, hence that part got checked too.
The analizer gave several messages which might be useful.
Error message: Common realloc mistake: 'response_buffer' nulled but not freed upon failure
The message reminds: realloc will return null pointer in case of an error, but current code does not verify if the pointer is valid.
That was in minihttptestserver.c, lines 417 and 418,
also in miniwget.c, lines 101, 241, 265.
Warning message: Assignment of function parameter has no effect outside the function. Did you forget dereferencing it?
That was in upnpcommands.c, line 162
The code was always like that, but most probably it was intended to zero uptime variable, and the line should have been
*uptime = 0; |
|
Back to top |
|
|
miniupnp Site Admin
Joined: 14 Apr 2007 Posts: 1589
|
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Sun Feb 08, 2015 1:57 am Post subject: |
|
|
Nice. It was quite impressive to get a new version in one day.
By the way, you might like to try using cppcheck yourself, it is simple to use and sometimes gives valueable hints.
There is a minor issue in testigddescparse.c, line 165 though it qualifies as an error: Memory leak: buffer
It could be fixed with free(buffer); before return 1; operator.
Even smaller is a similar issue in wingenminiupnpcstrings.c, line 62 - error Resource leak: fin
It would be sufficient to add fclose(fin); before return 1; operator. |
|
Back to top |
|
|
miniupnp Site Admin
Joined: 14 Apr 2007 Posts: 1589
|
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Sun Feb 08, 2015 2:56 pm Post subject: |
|
|
It was a big cleanup day for the code.
I also pay attention to less severe messages (restricting variable scope, etc.), but that is up to you.
A github question.
I clicked on your link, then on Browse files and then on Download zip
I see new files in browser, but zip contains mostly old versions of files.
For example, upnpreplyparse.c :
in miniupnpc-1.9.20150206.tar.gz I already have
Quote: | /* $Id: upnpreplyparse.c,v 1.18 2014/11/05 05:36:08 nanard Exp $ */ |
but in zip:
Quote: | /* $Id: upnpreplyparse.c,v 1.17 2014/11/04 22:25:48 nanard Exp $ */ |
I changed branch to master - still mostly old files.
Am I missing something or it is a github issue? |
|
Back to top |
|
|
miniupnp Site Admin
Joined: 14 Apr 2007 Posts: 1589
|
Posted: Mon Feb 09, 2015 8:49 am Post subject: |
|
|
don't pay attention to cvs headers
github files are always the most recent ones: ) _________________ Main miniUPnP author.
https://miniupnp.tuxfamily.org/ |
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Sat Feb 21, 2015 6:11 pm Post subject: |
|
|
Thank you; so it was git feature.
Today I played with cppcheck again and saw about 25 portabiliy messages in upnpcommands.c, starting from line 160:
Code: | scanf without field width limits can crash with huge input data on some versions of libc. |
What would you say, should those be fixed? |
|
Back to top |
|
|
miniupnp Site Admin
Joined: 14 Apr 2007 Posts: 1589
|
Posted: Mon Feb 23, 2015 9:06 am Post subject: |
|
|
fox88 wrote: | Thank you; so it was git feature.
Today I played with cppcheck again and saw about 25 portabiliy messages in upnpcommands.c, starting from line 160:
Code: | scanf without field width limits can crash with huge input data on some versions of libc. |
What would you say, should those be fixed? |
my cppcheck 1.54 doesn't tell me anything.
I don't know how to fix that _________________ Main miniUPnP author.
https://miniupnp.tuxfamily.org/ |
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Mon Feb 23, 2015 6:51 pm Post subject: |
|
|
miniupnp wrote: | What would you say, should those be fixed? |
It is about adding maximum width specifier into format string.
For example, %u could be replaced with %9u.
miniupnp wrote: | my cppcheck 1.54 doesn't tell me anything. |
Ancient one. Get current version 1.68 at http://cppcheck.sourceforge.net/. |
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Sat Mar 21, 2015 11:02 am Post subject: |
|
|
Another checking, this time with VS2013 built-in analyzer.
It found 40 suspicious places in code, but those are mostly of two types.
Example of the first type - ignored return value from sscanf:
Code: | C6031 Return value ignored Return value ignored: 'sscanf'. miniupnpc upnpcommands.c 160
|
Example of the second type - no checks for malloc/calloc returned pointer.
Code: | C6011 Dereferencing null pointer Dereferencing NULL pointer 'AddPortMappingArgs'. miniupnpc upnpcommands.c 357
'AddPortMappingArgs' may be NULL 356
'AddPortMappingArgs' is dereferenced, but may still be NULL 357
|
You could try searching through all the code for sscanf, malloc and calloc respectively if you would like to make changes.
Or try free VS 2013 Community edition to verify libraries yourself. |
|
Back to top |
|
|
miniupnp Site Admin
Joined: 14 Apr 2007 Posts: 1589
|
Posted: Mon Mar 23, 2015 9:41 am Post subject: |
|
|
there are a lot of sscanf which are not checked _________________ Main miniUPnP author.
https://miniupnp.tuxfamily.org/ |
|
Back to top |
|
|
fox88
Joined: 05 Feb 2015 Posts: 9
|
Posted: Mon Jul 27, 2015 3:13 pm Post subject: |
|
|
You really should get recent cppcheck and use it regularly.
In the latest version of the library:
Code: | content_buf = malloc(content_buf_len);
if(header_buf == NULL)
{
free(header_buf);
|
Of course, content_buf must be checked for NULL instead (file miniwget.c, line 97).
Also buffer is not freed at line 168 in file testigddescparse.c . |
|
Back to top |
|
|
techmania Guest
|
Posted: Mon Sep 03, 2018 7:54 pm Post subject: |
|
|
I have just check it found it is cause for my system |
|
Back to top |
|
|
|