buglwIP - A Lightweight TCP/IP stack - Bugs: bug #59831, tcp_output : Null dereferencing

 
 

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

bug #59831: tcp_output : Null dereferencing

Submitter:  Jonathan D <jona>
Submitted:  Wed 06 Jan 2021 10:22:43 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  git head

Jump to the original submission

Mon 18 Jan 2021 09:37:29 PM UTC, comment #7: 

comment #5:

> Comment #2 makes me think a bit, but this at least seems to be a different bug than what the OP saw?


It's possible that this is related, however what I'm seeing is also causing this issue here.

If you take a look at the tcp_out.c file:
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/tcp_out.c#n1328
  useg = pcb->unacked;
  if (useg != NULL) {

useg is beign set to pcb->unacked. Following this, there is a check for null, which implies that useg can be null.

On line 1382, we then check pcb->unacked for null, and not useg for null. These should be the same, so long as pcb was not modified by another thread.
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/tcp_out.c#n1390

So, the linters are only triggering this because two different variable names are being used and checked for null. I think if the following line checked useg instead, then the linter would be happy:

Note that this would also reduce the probability of the issue being triggered by the OP, but would not necessarily fix their issue.

After that analysis, I would agree that these are two seperate issues
1) the way the code is written is causing confusion to both pclint and clang-analyzer
2) The OP is running into an because multiple threads accessing tcp routines that modify internals (presumable).

Evan Lojewski <meklort>
Mon 18 Jan 2021 09:09:25 PM UTC, comment #6: 

Yes, the problem is not that FreeRTOS does task switching, the problem is that other tasks access tcp internals.

See common pitfalls here: https://www.nongnu.org/lwip/2_1_x/pitfalls.html

Are you sure your other tasks behave correctly?

Erik Ekman <yarrick>
Group Member
Mon 18 Jan 2021 08:51:00 PM UTC, comment #5: 

Re comment #3:
Right. I would have closed this bug right away after reading the original post (sorry I keep finding no time for lwIP lately :-/). There's no way another task can run between those lines and modify lwIP variables. If it does, threading requirements are violated and the OP should read our docs.

Comment #2 makes me think a bit, but this at least seems to be a different bug than what the OP saw?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jan 2021 08:27:51 PM UTC, comment #4: 

By "Another tasks was executed between this line 1328 and line 1382", I mean that FreeRTOS can switch tasks when the time allowed for one tasks is elapsed : https://www.freertos.org/implementation/a00005.html

Always talking about FreeRTOS, yes, there is a way to prevent tasks switching by surrunding the porting with taskENTER_CRITICAL() and  taskEXIT_CRITICAL() : https://www.freertos.org/taskENTER_CRITICAL_taskEXIT_CRITICAL.html

Jonathan D <jona>
Mon 18 Jan 2021 08:00:49 PM UTC, comment #3: 

What do you mean that "Another tasks was executed between this line 1328 and line 1382" ?

I don't know the details of how lwIP runs with an RTOS, but this seems like a violation of the concurrency expectations. Is there anything to stop another task from running in between the new location of useg setup and its dereference?


Erik Ekman <yarrick>
Group Member
Mon 18 Jan 2021 05:25:23 AM UTC, comment #2: 

I've also run into this issue when attempting to integrate lwip into one of my projects.

Also note that both scan-build from clang and pclint are detecting that a null deference is present at https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/tcp_out.c#n1390

From scan-build:
 ../libs/lwip/lwip/src/core/tcp_out.c:1390:67: warning: Access to field 'tcphdr' results in a dereference of a null pointer (loaded from variable 'useg')
        if (TCP_SEQ_LT(lwip_ntohl(seg->tcphdr->seqno), lwip_ntohl(useg->tcphdr->seqno))) {
                                                                  ^~~~~~~~~~~~
../libs/lwip/lwip/src/include/lwip/def.h:103:34: note: expanded from macro 'lwip_ntohl'
#define lwip_ntohl(x) lwip_htonl(x)
                                 ^
../libs/lwip/lwip/src/include/lwip/priv/tcp_priv.h:109:60: note: expanded from macro 'TCP_SEQ_LT'
#define TCP_SEQ_LT(a,b)     (((u32_t)((u32_t)(a) - (u32_t)(b)) & 0x80000000u) != 0)



From pclint:
        if (TCP_SEQ_LT(lwip_ntohl(seg->tcphdr->seqno), lwip_ntohl(useg->tcphdr->seqno))) {
../libs/lwip/lwip/src/core/tcp_out.c  1390  Warning 613: Possible
    use of null pointer 'useg' in left argument to operator '->' [Reference:
    file ../libs/lwip/lwip/src/core/tcp_out.c: line 1329]
../libs/lwip/lwip/src/core/tcp_out.c  1329  Info 831: Reference
    cited in prior message

Evan Lojewski <meklort>
Wed 06 Jan 2021 10:24:53 AM UTC, comment #1: 

The crash looks also very similar to the one described there :
http://lwip.100.n7.nabble.com/Crash-in-tcp-output-td36132.html

Jonathan D <jona>
Wed 06 Jan 2021 10:22:43 AM UTC, original submission:  

We are developing a device with LwIP and FreeRTOS.

We tested freemodbus and run an endurance test with a polling to modbus every second.

After some hours (we were able to reproduce the issue), the device crashed with a Null dereferencing for `useg` at this line :
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/tcp_out.c#n1390

Our assumption is that `pcb->unacked` was null at the moment it was copied to `useg`. Another tasks was executed between this line 1328 and line 1382. This other task filled `pcb->unacked` and this lead to the crash described above.

The attached patch has been tested successfully so far.

Jonathan D <jona>

 

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

Attached Files
file #50666:  LwIP_null_dereferencing_crash.diff added by jona (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by meklort (Posted a comment)
  • -email is unavailable- added by jona (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.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-06 jona Attached File- Added LwIP_null_dereferencing_crash.diff, #50666

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code