the-tcpdump-group / libpcap

the LIBpcap interface to various kernel packet capture mechanism
https://www.tcpdump.org/
Other
2.72k stars 854 forks source link

Double-free in daemon.c #794

Closed gvanem closed 5 years ago

gvanem commented 5 years ago

I experience frequent crashes in rpcapd.exe when pressing ^C to stop a windump-client. It seems there is a double call to pcap_close(); last statement in daemon.c. This session_close() is duplicated in 2 other places (rather messy and confusing).

Building everything in _DEBUG-mode clearly shows p->opt.device (in pcap_close()) is filled with 0xDDDD.. at the time of the crash.

Doing this patch seems to plug this double-free:

--- a/daemon.c 2019-01-09 15:42:15
+++ b/daemon.c 2019-01-09 20:27:39
@@ -1927,6 +1927,7 @@
        {
                if (session->fp)
                        pcap_close(session->fp);
+               session->fp = NULL;
 #ifdef HAVE_OPENSSL
                if (session->ctrl_ssl)
                        SSL_free(session->ctrl_ssl);
@@ -2005,6 +2006,7 @@
        {
                if (session->fp)
                        pcap_close(session->fp);
+               session->fp = NULL;
 #ifdef HAVE_OPENSSL
                if (session->ctrl_ssl)
                        SSL_free(session->ctrl_ssl);
@@ -2706,5 +2708,7 @@
                session->sockdata = 0;
        }

+       if (session->fp)
             pcap_close(session->fp);
+       session->fp = NULL;
 }

But I fail to understand fully why (?)

guyharris commented 5 years ago

Well, there are some confusing bits there.

Closing the pcap_t is one thing; that should be done when you're done capturing.

But that shouldn't be mixed with finishing up SSL on the control socket; that should be done right before the control socket is closed.

I'll look at cleaning that up.

guyharris commented 5 years ago

OK, so I did some cleanups in 5ec780d84809c8be1398f55527ada06f4ec7034e, e6715e9568197af6c1f8ce010c2f4682581db621, and 6271a52c71bc573b243a7016f472bc075bd5cd78.

gvanem commented 5 years ago

OK, so I did some cleanups in

Applying all those, it seems very stable. No crashes in _DEBUG so far.