buglwIP - A Lightweight TCP/IP stack - Bugs: bug #36492, Static Analysis on code 1.4.0

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #36492: Static Analysis on code 1.4.0

Submitter:  bayard <bayard>
Submitted:  Fri 18 May 2012 12:02:22 PM UTC
   
 
Category:  Contrib Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.0

Jump to the original submission

Sun 06 Apr 2014 07:44:22 PM UTC, comment #12: 

Done, but most of the warnings were either port related or false positives...
Thanks anyway, 1 or 2 were actually meaningful!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jun 2012 07:08:31 PM UTC, comment #11: 

This will probably get addressed for the next major release.  I'd rather not delay the 1.4.1 release while we wait to analyse the errors.

Kieran Mansley <kieranm>
Group Member
Sun 10 Jun 2012 08:54:13 AM UTC, comment #10: 

Any news ?

bayard <bayard>
Wed 30 May 2012 04:16:04 PM UTC, comment #9: 

I modified debug.h on my side with an infinite loop in LWIP_ASSERT(). The number of error decreased: only 13 !

Please find below debug.h file.

Errors are here

ABR : Buffer overflow, array index of 'hwaddr' may be out of bounds. Array 'hwaddr' of size 6 may use index value(s) 6..15 : lwip/src/core/dhcp.c : 1683 : Critical : Analyze
ABV.STACK : Buffer overflow, array index of 'EventTbl' may be out of bounds. Array 'EventTbl' of size 30 may use index value(s) 0..254 : lwip/src/api/api_lib.c : 86 : Critical : Analyze
INFINITE_LOOP.LOCAL : Infinite loop : lwip/src/api/tcpip.c : 87 : Error : Analyze
NPD.CHECK.MIGHT : Pointer 'to' checked for NULL at line 818 may be dereferenced at line 820. : lwip/src/api/sockets.c : 820 : Critical : Analyze
NPD.FUNC.MUST : Pointer 'frame.descriptor' returned from call to function 'ETH_RxPkt_ChainMode' at line 236 may be NULL and will be dereferenced at line 257. : lwip/port/ethernetif/ethernetif.c : 257 : Critical : Analyze
NPD.FUNC.MUST : Pointer 'msg' returned from call to function 'sys_timeouts_mbox_fetch' at line 91 may be NULL and will be dereferenced at line 93. : lwip/src/api/tcpip.c : 93 : Critical : Analyze
RNPD.DEREF : Suspicious dereference of pointer 'ipr->p' before NULL check at line 444 : lwip/src/core/ipv4/ip_frag.c : 426 : Critical : Analyze
RNPD.DEREF : Suspicious dereference of pointer 'conn->current_msg' before NULL check at line 1207 : lwip/src/api/api_msg.c : 1203 : Critical : Analyze
UNINIT.STACK.ARRAY.MIGHT : 'parts' array elements might be used uninitialized in this function. Also there are 2 similar errors on line(s) 233, 240. : lwip/src/core/ipv4/ip_addr.c : 226 : Critical : Analyze
UNINIT.STACK.MIGHT : 'port' might be used uninitialized in this function. : lwip/src/api/sockets.c : 674 : Critical : Analyze
UNINIT.STACK.MUST : 'naddr.addr' is used uninitialized in this function. : lwip/src/api/sockets.c : 1418 : Critical : Analyze
UNINIT.STACK.MUST : 'port' is used uninitialized in this function. : lwip/src/api/sockets.c : 363 : Critical : Analyze
UNINIT.STACK.MUST : 'naddr.addr' is used uninitialized in this function. : lwip/src/api/sockets.c : 364 : Critical : Analyze


(file #25958)

bayard <bayard>
Tue 29 May 2012 08:16:11 PM UTC, comment #8: 


> NPD.CHECK.CALL.MUST : Pointer 'prev' checked for NULL at line 166 will
> be passed to function and may be dereferenced there by passing
> argument 2 to function 'ip_reass_dequeue_datagram' at line 203. :
> /home/g133009/px100/Code/lwip/src/core/ipv4/ip_frag.c : 203 :
> Critical : Analyze


And that's another problem of the static analyzer: LWIP_ASSERT is used in function 'ip_reass_dequeue_datagram' to ensure the pointer is not null (as in this case, the condition 'reassdatagrams == ipr' whold be true). I guess you did not make LWIP_ASSERT correctly known as assert function to your static analysis tool (however that may work).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 29 May 2012 08:13:12 PM UTC, comment #7: 


> What tool are you using to produce these reports?


One of the Klocwork programs, from asking google...

> Neither 'port' nor 'naddr.addr' are directly initialized in the
> function lwip_accept().
>
> If functions called by lwip_accept() initialized the variables
> it should be OK, but there is no guaranty.


And that's the problem with static analysis: this will never fail, as we do know that netconn_peer() always initializes these variables if it returns ERR_OK (and if it doesn't return ERR_OK, line 363 is not reached).

Nevertheless, I regard static analysis as input worth checking, so I'll have a look at this, thanks for updating the report.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 25 May 2012 12:16:14 PM UTC, comment #6: 

Neither 'port' nor 'naddr.addr' are directly initialized in the function lwip_accept().

If functions called by lwip_accept() initialized the variables it should be OK, but there is no guaranty.

bayard <bayard>
Thu 24 May 2012 12:09:37 PM UTC, comment #5: 


UNINIT.STACK.MUST :
'port' is used uninitialized in this function.
lwip/src/api/sockets.c : 363

UNINIT.STACK.MUST :
'naddr.addr' is used uninitialized in this function.
lwip/src/api/sockets.c : 364 :

I don't understand these two warnings.
The variables are set on line 347 by
err = netconn_peer(newconn, &naddr, &port);

What tool are you using to produce these reports?

Mason <mason>
Tue 22 May 2012 04:19:42 PM UTC, comment #4: 

Please find below the results with modification on macro.


ABR : Buffer overflow, array index of 'hwaddr' may be out of bounds.
Array 'hwaddr' of size 6 may use index value(s) 6..15 :
/home/g133009/px100/Code/lwip/src/core/dhcp.c : 1683 : Critical :
Analyze
INFINITE_LOOP.LOCAL : Infinite loop :
/home/g133009/px100/Code/lwip/src/api/tcpip.c : 87 : Error : Analyze
NPD.CHECK.CALL.MUST : Pointer 'prev' checked for NULL at line 166 will
be passed to function and may be dereferenced there by passing
argument 2 to function 'ip_reass_dequeue_datagram' at line 203. :
/home/g133009/px100/Code/lwip/src/core/ipv4/ip_frag.c : 203 : Critical
: Analyze
NPD.CHECK.MIGHT : Pointer 'p_to' checked for NULL at line 873 may be
dereferenced at line 845. :
/home/g133009/px100/Code/lwip/src/core/pbuf.c : 845 : Critical :
Analyze
NPD.CHECK.MIGHT : Pointer 'to' checked for NULL at line 818 may be
dereferenced at line 820. :
/home/g133009/px100/Code/lwip/src/api/sockets.c : 820 : Critical :
Analyze
NPD.FUNC.MIGHT : Pointer 'buf' returned from call to function
'sys_arch_mbox_fetch' at line 371 may be NULL and may be dereferenced
at line 407. : /home/g133009/px100/Code/lwip/src/api/api_lib.c : 407 :
Critical : Analyze
NPD.FUNC.MUST : Pointer 'frame.descriptor' returned from call to
function 'ETH_RxPkt_ChainMode' at line 236 may be NULL and will be
dereferenced at line 257. :
/home/g133009/px100/Code/lwip/port/ethernetif/ethernetif.c : 257 :
Critical : Analyze
NPD.FUNC.MUST : Pointer 'q' returned from call to function 'mem_trim'
at line 450 may be NULL and will be dereferenced at line 454. :
/home/g133009/px100/Code/lwip/src/core/pbuf.c : 454 : Critical :
Analyze
NPD.FUNC.MUST : Pointer 'sock' returned from call to function
'tryget_socket' at line 1143 may be NULL and will be dereferenced at
line 1146. Also there is one similar error on line 1178. :
/home/g133009/px100/Code/lwip/src/api/sockets.c : 1146 : Critical :
Analyze
NPD.FUNC.MUST : Pointer 'msg' returned from call to function
'sys_timeouts_mbox_fetch' at line 91 may be NULL and will be
dereferenced at line 93. :
/home/g133009/px100/Code/lwip/src/api/tcpip.c : 93 : Critical :
Analyze
NPD.GEN.CALL.MIGHT : Null pointer 'prev' that comes from line 124 may
be passed to function and can be dereferenced there by passing
argument 2 to function 'ip_reass_free_complete_datagram' at line 143.
: /home/g133009/px100/Code/lwip/src/core/ipv4/ip_frag.c : 143 :
Critical : Analyze
NPD.GEN.CALL.MIGHT : Null pointer 'prev' that comes from line 234 may
be passed to function and can be dereferenced there by passing
argument 2 to function 'ip_reass_free_complete_datagram' at line 254.
: /home/g133009/px100/Code/lwip/src/core/ipv4/ip_frag.c : 254 :
Critical : Analyze
NPD.GEN.CALL.MIGHT : Null pointer 'ipr_prev' that comes from line 484
may be passed to function and can be dereferenced there by passing
argument 2 to function 'ip_reass_dequeue_datagram' at line 596. :
/home/g133009/px100/Code/lwip/src/core/ipv4/ip_frag.c : 596 : Critical
: Analyze
RNPD.DEREF : Suspicious dereference of pointer 'conn' before NULL
check at line 326 : /home/g133009/px100/Code/lwip/src/api/api_msg.c :
320 : Critical : Analyze
UNINIT.STACK.ARRAY.MIGHT : 'parts' array elements might be used
uninitialized in this function. Also there are 2 similar errors on
line(s) 233, 240. :
/home/g133009/px100/Code/lwip/src/core/ipv4/ip_addr.c : 226 : Critical
: Analyze
UNINIT.STACK.MIGHT : 'port' might be used uninitialized in this
function. : /home/g133009/px100/Code/lwip/src/api/sockets.c : 674 :
Critical : Analyze
UNINIT.STACK.MUST : 'naddr.addr' is used uninitialized in this
function. : /home/g133009/px100/Code/lwip/src/api/sockets.c : 1418 :
Critical : Analyze
UNINIT.STACK.MUST : 'port' is used uninitialized in this function. :
/home/g133009/px100/Code/lwip/src/api/sockets.c : 363 : Critical :
Analyze
UNINIT.STACK.MUST : 'naddr.addr' is used uninitialized in this
function. : /home/g133009/px100/Code/lwip/src/api/sockets.c : 364 :
Critical : Analyze

bayard <bayard>
Sun 20 May 2012 08:57:40 AM UTC, comment #3: 

I can only confirm what Mason wrote: there are far too many false positives in the file you provided. Could you please provide an updated one that correctly taks LWIP_ASSERT as a null-pointer check?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 18 May 2012 12:54:27 PM UTC, comment #2: 


> You need to find a way to tell your analyzer that LWIP_PLATFORM_ASSERT behaves like assert.


I was incorrect. LWIP_PLATFORM_ASSERT behaves like abort (NB: this macro should be renamed).

In GCC-speak, LWIP_PLATFORM_ASSERT would be given the "noreturn" attribute.

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

Mason <mason>
Fri 18 May 2012 12:33:10 PM UTC, comment #1: 

"Pointer 'foo' checked for NULL at line N will be dereferenced at line M" messages are likely all false positives, as line N is an LWIP_ASSERT, which shall not return if the assertion does not hold.

#ifndef LWIP_NOASSERT
#define LWIP_ASSERT(message, assertion) do { if(!(assertion)) \
  LWIP_PLATFORM_ASSERT(message); } while(0)
#else  /* LWIP_NOASSERT */
#define LWIP_ASSERT(message, assertion)
#endif /* LWIP_NOASSERT */

You need to find a way to tell your analyzer that LWIP_PLATFORM_ASSERT behaves like assert.

Mason <mason>
Fri 18 May 2012 12:02:22 PM UTC, original submission:  

Please find below a static analysis on code Lwip version 1.4.0.

bayard <bayard>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #25958:  debug.h added by bayard (4KiB - application/octet-stream - debug.h modified for static analysis)
file #25880:  PotentialBugsLWIP.txt added by bayard (10KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by mason (Posted a comment)
  • -email is unavailable- added by bayard (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-04-06 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2012-05-30 bayard Attached File- Added debug.h, #25958
    2012-05-18 bayard Attached File- Added PotentialBugsLWIP.txt, #25880

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code