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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
URL:
  <https://savannah.nongnu.org/bugs/?57377>

                 Summary: Assertion "pbuf_free: p->ref > 0" failed
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: vhertz
            Submitted on: Sat 07 Dec 2019 01:17:27 PM UTC
                Category: TCP
                Severity: 3 - Normal
              Item Group: Crash Error
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: Other

    _______________________________________________________

Details:

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




    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #1, bug #57377 (project lwip):

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)
    _______________________________________________________

Additional Item Attachment:

File name: triple_fuzz.patch              Size:16 KB
    <https://savannah.nongnu.org/file/triple_fuzz.patch?file_id=49312>

File name: crashed_inputs.tgz             Size:9 KB
    <https://savannah.nongnu.org/file/crashed_inputs.tgz?file_id=49313>



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #2, bug #57377 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #3, bug #57377 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Update of bug #57377 (project lwip):

                  Status:                    None => Fixed                  
             Assigned to:                    None => yarrick                
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #4:

Fixed in
http://git.savannah.nongnu.org/cgit/lwip.git/commit/?id=e80d4ff2cc5f8f864e9e996c72b47ebefd2a5175

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #5, bug #57377 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #6, bug #57377 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ashley Duncan
Follow-up Comment #7, bug #57377 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57377>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel