miniupnp.tuxfamily.org Forum Index miniupnp.tuxfamily.org
The forum about miniupnp and libnatpmp
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

Checking miniupnpc code

 
Post new topic   Reply to topic    miniupnp.tuxfamily.org Forum Index -> miniupnpc Bugs
View previous topic :: View next topic  
Author Message
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Thu Feb 05, 2015 6:57 pm    Post subject: Checking miniupnpc code Reply with quote

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
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1589

PostPosted: Fri Feb 06, 2015 10:40 am    Post subject: Reply with quote

Thank you for reporting theses issues

https://github.com/miniupnp/miniupnp/commit/557fd71fb1407c5de53cd9a19f167c70488077d2

https://github.com/miniupnp/miniupnp/commit/e280c3dd3abb7fd636c06503d0f7041256717387
_________________
Main miniUPnP author.
https://miniupnp.tuxfamily.org/
Back to top
View user's profile Send private message Visit poster's website
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Sun Feb 08, 2015 1:57 am    Post subject: Reply with quote

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
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1589

PostPosted: Sun Feb 08, 2015 9:41 am    Post subject: Reply with quote

I used cppcheck Smile
https://github.com/miniupnp/miniupnp/commit/c7d7efd2302b561e0d321c7e9c426396174bf6cc
_________________
Main miniUPnP author.
https://miniupnp.tuxfamily.org/
Back to top
View user's profile Send private message Visit poster's website
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Sun Feb 08, 2015 2:56 pm    Post subject: Reply with quote

It was a big cleanup day for the code. Smile
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
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1589

PostPosted: Mon Feb 09, 2015 8:49 am    Post subject: Reply with quote

don't pay attention to cvs headers Wink
github files are always the most recent ones: )
_________________
Main miniUPnP author.
https://miniupnp.tuxfamily.org/
Back to top
View user's profile Send private message Visit poster's website
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Sat Feb 21, 2015 6:11 pm    Post subject: Reply with quote

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
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1589

PostPosted: Mon Feb 23, 2015 9:06 am    Post subject: Reply with quote

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
View user's profile Send private message Visit poster's website
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Mon Feb 23, 2015 6:51 pm    Post subject: Reply with quote

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
View user's profile Send private message
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Sat Mar 21, 2015 11:02 am    Post subject: Reply with quote

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
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1589

PostPosted: Mon Mar 23, 2015 9:41 am    Post subject: Reply with quote

there are a lot of sscanf which are not checked Sad
_________________
Main miniUPnP author.
https://miniupnp.tuxfamily.org/
Back to top
View user's profile Send private message Visit poster's website
fox88



Joined: 05 Feb 2015
Posts: 9

PostPosted: Mon Jul 27, 2015 3:13 pm    Post subject: Reply with quote

You really should get recent cppcheck and use it regularly. Smile

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
View user's profile Send private message
techmania
Guest





PostPosted: Mon Sep 03, 2018 7:54 pm    Post subject: Reply with quote

I have just check it found it is cause for my system
Back to top
Display posts from previous:   
Post new topic   Reply to topic    miniupnp.tuxfamily.org Forum Index -> miniupnpc Bugs All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group
Protected by Anti-Spam ACP
© 2007 Thomas Bernard, author of MiniUPNP.