search

the-tcpdump-group / libpcap

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

Call WSAStartup and WSACleanup for each use. Fixes #1333 #1351

Closed dmiller-nmap closed 2 months ago

dmiller-nmap commented 2 months ago

WSACleanup cannot be called from atexit() because that's run in DllMain context. Instead, start and cleanup at each use, using existing reference counting functions from sockutils.c.

Performance impact ought to be small because these are not frequently-called functions in the lifetime of a capture handle, and because WSAStartup/WSACleanup are essentially just reference increment/decrement in the common case of an application that has already called WSAStartup.

I believe this commit covers all likely situations, with one unlikely exception: the functions in nametoaddr.c are exported by the library, though they are not documented in the manpages. They are only called internally from functions within gencode.c which should be handled by the startup/cleanup calls in pcap_compile(), and I have not found any applications on Github that call them directly. If any application had previously called them directly, relying on pcap_init() to call WSAStartup(), then that code would need to be changed to call WSAStartup() itself. I couldn't just add startup/cleanup within each function in nametoaddr.c because several of them return static buffers allocated by Winsock functions, which I assume could be invalidated by calling WSACleanup().

guyharris commented 2 months ago

using existing reference counting functions from sockutils.c

libpcap, even on Windows, can be compiled without remote capture support. It defaults to building with that support on Windows, because WinPcap had it, but it should support building without it.

So perhaps we should have static inline pcapint_sockinit() and pcapint_sockcleanup() functions in portability.h that call WSAStartup() and WSACleanup(), respectively, on Windows, and do nothing elsewhere ("elsewhere" includes Haiku as well as UN*Xes). Those function would be the same as sock_init() and sock_cleanup(), except that they wouldn't maintain their own use count, but would unconditionally call WSAStartup() and WSACleanup(), and would replace sock_init() and sock_cleanup().

As for the nametoaddr.c functions, as per your suggestion, let's let pcap_compile() handle the initialization for them for now. I might be tempted to deprecate those functions, with a recommendation to "just do it yourself", to try to flush out any programs that use them.

infrastation commented 2 months ago

Before merging please squash into one commit and add a change log entry. If the changes to the man pages stand, please bump the last modified date at the top and see if the "backward compatibility" section needs an update.

dmiller-nmap commented 2 months ago

I believe this is ready for review again. The linux-armv7l build failure does not appear to be related to my changes.

dmiller-nmap commented 2 months ago

Comment updated. The build errors look like network transient errors. I confirmed that the Npcap SDK is accessible.

guyharris commented 2 months ago

The build errors look like network transient errors. I confirmed that the Npcap SDK is accessible.

Appveyor is... not the most reliable build server. It frequently has a problem either downloading stuff from Chocolatey or downloading the Npcap SDK; I've restarted the build for this patch request (as well as the build for another patch request or change to tcpdump)..