buglwIP - A Lightweight TCP/IP stack - Bugs: bug #57377, Assertion "pbuf_free:...

 
 

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

bug #57377: Assertion "pbuf_free: p->ref > 0" failed

Submitted by:  Hiromasa Ito <vhertz>
Submitted on:  Sat 07 Dec 2019 01:17:27 PM UTC  
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  Erik Ekman <yarrick>
Open/Closed:  Closed Planned Release:  None
lwIP version:  Other

( Jump to the original submission)

Sun 21 Jun 2020 08:28:59 PM UTC, comment #7: 

Using latest git code:

$ cd test/fuzz
$ make all D="-DLWIP_RAND_FOR_FUZZ_SIMULATE_GLIBC"
$ for i in `ls crashed_inputs/*`; do ./lwip_fuzz3 $i; done
reading input from file... testing file: "crashed_inputs/001"...
reading input from file... testing file: "crashed_inputs/002"...
reading input from file... testing file: "crashed_inputs/003"...
reading input from file... testing file: "crashed_inputs/004"...
Assertion "mss_local is too small" failed at line 486 in ../../src/core/tcp_out.c
Aborted (core dumped)
reading input from file... testing file: "crashed_inputs/005"...
Assertion "inconsistent oversize vs. space" failed at line 504 in ../../src/core/tcp_out.c
Aborted (core dumped)
reading input from file... testing file: "crashed_inputs/006"...
reading input from file... testing file: "crashed_inputs/007"...
Assertion "tcp_slowtimr: persist ticking with empty send buffer" failed at line 1241 in ../../src/core/tcp.c
Aborted (core dumped)
reading input from file... testing file: "crashed_inputs/008"...
Assertion "Can't split segment into length 0" failed at line 851 in ../../src/core/tcp_out.c
Aborted (core dumped)
reading input from file... testing file: "crashed_inputs/009"...

So after this one was fixed (006) there are still 4 left that crash. I am not familiar with the tcp code so I am not sure if I can fix them.

Erik Ekman <yarrick>
Project MemberIn charge of this item.
Sun 21 Jun 2020 08:19:15 PM UTC, comment #6: 

I did not write it here, but setting the LWIP_RAND_FOR_FUZZ_SIMULATE_GLIBC define allows LWIP_RAND_FOR_FUZZ to be kept. It will then return the first 20 rand() values which would have been used before with normal LWIP_RAND.

Erik Ekman <yarrick>
Project MemberIn charge of this item.
Sun 21 Jun 2020 01:25:51 PM UTC, comment #5: 

> Did I miss something or did some other fuzz setting change?

As far as I know, my test driver is merged, refactored, and improved by Simon (@goldsimon). As a result of the process of them, some crashes I reported seem to become unreproducible.

As Erik (@yarrick) written, LWIP_RAND_FOR_FUZZ seems to make them unreproducible. Local ports of TCP/UDP clients depend on LWIP_RAND(), and in the crashed test cases these numbers are hard-coded (TCP: 58311, UDP: 50536). As LWIP_RAND_FOR_FUZZ allows to override LWIP_RAND(), I think this may be why the crashes have become unreproducible.

Hiromasa Ito <vhertz>
Fri 19 Jun 2020 01:40:38 PM UTC, comment #4: 
Erik Ekman <yarrick>
Project MemberIn charge of this item.
Fri 19 Jun 2020 12:55:35 PM UTC, comment #3: 

I agree that this looks like a double free.

tcp_slowtmr: processing active pcb
pbuf_alloc(length=1)
pbuf_alloc(length=1) == 0x43d816
tcp_create_segment: no memory.
pbuf_free(0x43d816)
pbuf_free: deallocating 0x43d816
tcp_split_unsent_seg: could not create new TCP segment
pbuf_free(0x43d816)
Assertion "pbuf_free: p->ref > 0" failed at line 753 in ../../src/core/pbuf.c

tcp_create_segment() frees the pbuf pointer, but this is not noticed by the caller since the pbuf was sent as a normal pointer (not pointer to pointer):

static struct tcp_seg *
tcp_create_segment(const struct tcp_pcb *pcb, struct pbuf *p, u8_t hdrflags, u32_t seqno, u8_t optflags);

Callers to tcp_create_segment:

tcp_write() line 666:
- Does not free the pbuf on error

tcp_split_unsent_seg() line 914:
- Does free pbuf on error (the case found in this bug)

tcp_enqueue_flags() line 1085:
- Does not free the pbuf on error

It seems the standard is to expect that the pbuf is freed, and tcp_split_unsent_seg() should be updated to not try to free it again.

My proposed fix would be:

diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
index bfb033b1..d9d1b57b 100644
--- a/src/core/tcp_out.c
+++ b/src/core/tcp_out.c
@@ -913,6 +913,7 @@ tcp_split_unsent_seg(struct tcp_pcb *pcb, u16_t split)

   seg = tcp_create_segment(pcb, p, remainder_flags, lwip_ntohl(useg->tcphdr->seqno) + split, optflags);
   if (seg == NULL) {
+    p = NULL; /* Freed by tcp_create_segment */
     LWIP_DEBUGF(TCP_OUTPUT_DEBUG | LWIP_DBG_LEVEL_SERIOUS,
                 ("tcp_split_unsent_seg: could not create new TCP segment\n"));
     goto memerr;

which removes the double free, and makes the test input not crash.

Supplying a pointer to pointer to tcp_create_segment is more work, and there are many ways the pbuf can be freed (like via tcp_seg_free, so it would be easy to miss resetting the pbuf pointer).

Erik Ekman <yarrick>
Project MemberIn charge of this item.
Fri 19 Jun 2020 11:14:22 AM UTC, comment #2: 

By disabling LWIP_RAND_FOR_FUZZ I got some of them to fail:
$ for i in `ls crashed_inputs/*`; do ./triple_fuzz $i ; echo $?; done
Reading input from file... testing file: "crashed_inputs/001"...
0
Reading input from file... testing file: "crashed_inputs/002"...
0
Reading input from file... testing file: "crashed_inputs/003"...
0
Reading input from file... testing file: "crashed_inputs/004"...
Assertion "mss_local is too small" failed at line 486 in ../../src/core/tcp_out.c
Aborted (core dumped)
134
Reading input from file... testing file: "crashed_inputs/005"...
Assertion "inconsistent oversize vs. space" failed at line 504 in ../../src/core/tcp_out.c
Aborted (core dumped)
134
Reading input from file... testing file: "crashed_inputs/006"...
Assertion "pbuf_free: p->ref > 0" failed at line 753 in ../../src/core/pbuf.c
Aborted (core dumped)
134
Reading input from file... testing file: "crashed_inputs/007"...
Assertion "tcp_slowtimr: persist ticking with empty send buffer" failed at line 1241 in ../../src/core/tcp.c
Aborted (core dumped)
134
Reading input from file... testing file: "crashed_inputs/008"...
Assertion "Can't split segment into length 0" failed at line 851 in ../../src/core/tcp_out.c
Aborted (core dumped)
134
Reading input from file... testing file: "crashed_inputs/009"...
0

Erik Ekman <yarrick>
Project MemberIn charge of this item.
Fri 19 Jun 2020 11:03:27 AM UTC, comment #1: 

I tried rebasing your lwIP patch to run with the latest code, see attachment. For me all the crashed_input files work when I try them. Did I miss something or did some other fuzz setting change?

$ for i in `ls crashed_inputs/*`; do ./triple_fuzz $i ; echo $?; done
Reading input from file... testing file: "crashed_inputs/001"...
0
Reading input from file... testing file: "crashed_inputs/002"...
0
Reading input from file... testing file: "crashed_inputs/003"...
0
Reading input from file... testing file: "crashed_inputs/004"...
0
Reading input from file... testing file: "crashed_inputs/005"...
0
Reading input from file... testing file: "crashed_inputs/006"...
0
Reading input from file... testing file: "crashed_inputs/007"...
0
Reading input from file... testing file: "crashed_inputs/008"...
0
Reading input from file... testing file: "crashed_inputs/009"...
0

(file #49312, file #49313)

Erik Ekman <yarrick>
Project MemberIn charge of this item.
Sat 07 Dec 2019 01:17:27 PM UTC, original submission:  

Hi, all.

This is one of the assertion failures I found by fuzzing (to lwIP ver2.1.0.RC1).
The following LWIP_ASSERT() at lwip/src/core/pbuf.c:753 fails.

LWIP_ASSERT("pbuf_free: p->ref > 0", p->ref > 0);

From my point of view, double-free of `p` causes this failure.
The following code is in tcp_split_unsent_seg().

seg = tcp_create_segment(pcb, p, remainder_flags, lwip_ntohl(useg->tcphdr->seqno) + split, optflags);
  if (seg == NULL) {
    LWIP_DEBUGF(TCP_OUTPUT_DEBUG | LWIP_DBG_LEVEL_SERIOUS,
                ("tcp_split_unsent_seg: could not create new TCP segment\n"));
    goto memerr;
}

tcp_create_segment() calls pbuf_free() if no memory to allocate, and it returns NULL.

memerr:
  TCP_STATS_INC(tcp.memerr);

  LWIP_ASSERT("seg == NULL", seg == NULL);
  if (p != NULL) {
    pbuf_free(p);
  }

If tcp_create_segment() has called pbuf_free(), this is 2nd pbuf_free() call.
As a result, the assertion fails.

You can reproduce this failure with 'crashed_inputs/006' attached to the following message of lwip-devel:
https://lists.nongnu.org/archive/html/lwip-devel/2019-12/msg00013.html

Hiromasa Ito <vhertz>

 

Attached Files
file #49312:  triple_fuzz.patch added by yarrick (16KiB - text/x-patch)
file #49313:  crashed_inputs.tgz added by yarrick (9KiB - application/x-compressed-tar)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by yarrick (Updated the item)
  • -email is unavailable- added by vhertz (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-06-19 yarrick StatusNone => Fixed
        Assigned toNone => yarrick
        Open/ClosedOpen => Closed
    2020-06-19 yarrick Attached File- => Added triple_fuzz.patch, #49312
        Attached File- => Added crashed_inputs.tgz, #49313

    Back to the top


    Powered by Savane 3.5