--- Begin Message ---
this patch fixes the use-after-free of `pd`, and also fixes the leak
of `device`. let me know if you need this uploaded to github
instead...
diff --git a/tcpdump.c b/tcpdump.c
index 658d8b34..4fa390fd 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -2239,8 +2239,12 @@ main(int argc, char **argv)
error("%s", pcap_geterr(pd));
if (dflag) {
bpf_dump(&fcode, dflag);
- pcap_close(pd);
+ /* Clear pd so our signal handler won't use-after-free it. */
+ pcap_t *to_free = pd;
+ pd = NULL;
+ pcap_close(to_free);
free(cmdbuf);
+ free(device);
pcap_freecode(&fcode);
exit_tcpdump(S_SUCCESS);
}
@@ -2559,7 +2563,10 @@ DIAG_ON_CLANG(assign-enum)
*/
info(1);
}
- pcap_close(pd);
+ /* Clear pd so our signal handler won't use-after-free it. */
+ pcap_t *to_free = pd;
+ pd = NULL;
+ pcap_close(to_free);
if (VFileName != NULL) {
ret = get_next_file(VFile, VFileLine);
if (ret) {
@@ -2640,6 +2647,7 @@ DIAG_ON_CLANG(assign-enum)
PLURAL_SUFFIX(packets_captured));
free(cmdbuf);
+ free(device);
pcap_freecode(&fcode);
exit_tcpdump(status == -1 ? 1 : 0);
}
@@ -2692,7 +2700,8 @@ cleanup(int signo _U_)
* to do anything with standard I/O streams in a signal handler -
* the ANSI C standard doesn't say it is).
*/
- pcap_breakloop(pd);
+ if (pd != NULL)
+ pcap_breakloop(pd);
#else
/*
* We don't have "pcap_breakloop()"; this isn't safe, but
On Wed, Oct 14, 2020 at 2:28 PM enh <e...@google.com> wrote:
>
> i haven't reproduced it myself yet (though i'll try shortly) but we
> got an automated crash report from tcpdump on Android via
> [gwp-asan](https://developer.android.com/ndk/guides/gwp-asan).
>
> the bug is a use-after-free, specifically when pcap_breakloop() tries
> to write to the already-freed struct pcap_t. this happens if a signal
> is received during tcpdump shutdown (which is presumably why we
> haven't hit this more often on ASan/HWASan builds).
>
> i assume the fix is to disable the signal handlers before calling
> pcap_close() to free the struct pcap_t, but i thought i'd bring this
> up on the list before i (a) look at reproducing this locally and (b)
> send a patch...
diff --git a/tcpdump.c b/tcpdump.c
index 658d8b34..4fa390fd 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -2239,8 +2239,12 @@ main(int argc, char **argv)
error("%s", pcap_geterr(pd));
if (dflag) {
bpf_dump(&fcode, dflag);
- pcap_close(pd);
+ /* Clear pd so our signal handler won't use-after-free it. */
+ pcap_t *to_free = pd;
+ pd = NULL;
+ pcap_close(to_free);
free(cmdbuf);
+ free(device);
pcap_freecode(&fcode);
exit_tcpdump(S_SUCCESS);
}
@@ -2559,7 +2563,10 @@ DIAG_ON_CLANG(assign-enum)
*/
info(1);
}
- pcap_close(pd);
+ /* Clear pd so our signal handler won't use-after-free it. */
+ pcap_t *to_free = pd;
+ pd = NULL;
+ pcap_close(to_free);
if (VFileName != NULL) {
ret = get_next_file(VFile, VFileLine);
if (ret) {
@@ -2640,6 +2647,7 @@ DIAG_ON_CLANG(assign-enum)
PLURAL_SUFFIX(packets_captured));
free(cmdbuf);
+ free(device);
pcap_freecode(&fcode);
exit_tcpdump(status == -1 ? 1 : 0);
}
@@ -2692,7 +2700,8 @@ cleanup(int signo _U_)
* to do anything with standard I/O streams in a signal handler -
* the ANSI C standard doesn't say it is).
*/
- pcap_breakloop(pd);
+ if (pd != NULL)
+ pcap_breakloop(pd);
#else
/*
* We don't have "pcap_breakloop()"; this isn't safe, but
--- End Message ---
_______________________________________________
tcpdump-workers mailing list
tcpdump-workers@lists.tcpdump.org
https://lists.sandelman.ca/mailman/listinfo/tcpdump-workers
