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:
  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.

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 :

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 :

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-c54d.
    Corresponding source code