Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IO plugins for libpcap #914

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

mcr
Copy link
Member

@mcr mcr commented Mar 2, 2020

rebased upon master

@mcr mcr mentioned this pull request Mar 2, 2020
@mcr mcr requested a review from guyharris March 2, 2020 21:15
@mcr mcr force-pushed the raybellis-gzip-v2 branch 4 times, most recently from 8045922 to 6796dc9 Compare March 2, 2020 22:56
@mcr mcr added this to the next-release milestone Mar 2, 2020
pcap-ioplugin.c Outdated
goto fail;
}

#if HAVE_DLOPEN
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I think we should also depend upon WANT_IOPLUGIN, so that it can be compiled out, even if dlopen exists.

@@ -98,6 +98,22 @@ extern int pcap_utf_8_mode;
((ull & 0x000000000000ff00ULL) << 40) | \
((ull & 0x00000000000000ffULL) << 56)

/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this?

@@ -1042,6 +1042,23 @@ PCAP_API int pcap_remoteact_list(char *hostlist, char sep, int size,
PCAP_API int pcap_remoteact_close(const char *host, char *errbuf);
PCAP_API void pcap_remoteact_cleanup(void);

/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have a doc/ioplugin.md

#include "ftmacros.h"

#ifdef _WIN32
#include <pcap-stdinc.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no such header file; it was removed in 787f37e.

You need to directly include what's needed.

@@ -74,7 +74,7 @@ main(int argc, char **argv)
struct bpf_program fcode;
char ebuf[PCAP_ERRBUF_SIZE];
pcap_if_t *devlist;
int selectable_fd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is already in the main branch from commit fbb5e9e (and is unrelated to the I/O plugins).

@guyharris
Copy link
Member

This looks as if it's not up to date with all the main branch changes, so it probably needs to be rebased.

@fxlb
Copy link
Member

fxlb commented Nov 14, 2021

Rebased.

const pcap_ioplugin_t*
pcap_ioplugin_init(const char *name)
{
void *lib = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./pcap-ioplugin.c: In function 'pcap_ioplugin_init':
./pcap-ioplugin.c:159:8: warning: unused variable 'lib' [-Wunused-variable]
  159 |  void *lib = NULL;
      |        ^~~

Copy link
Member

@guyharris guyharris Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several issus here.

  1. On UN*X, we need either to assume we have the dlopen() APIs or check for it in CMakeLists.txt as well. It's specified in POSIX and, as far as I know, the only general-purpose UN*Xes that don't have it we're likely to encounter are 1) 32-bit HP-UX (which has its own API) and 2) perhaps older versions of AIX. However, perhaps some embedded Linux distributions have a cut-back libc that doesn't have it.
  2. We're not handling Windows which has its own APIs for this.
  3. If we don't have any way to load plugins, we shouldn't define a variable to hold the plugin handle.

@guyharris
Copy link
Member

If we support plugins for writing files, and you're on a platform where packet capture requires elevated privileges, and the user can specify a plugin with an arbitrary path name with an environment variable, every program that uses the pcap dumping APIs needs to be Very Careful to surrender all elevated privileges before doing anything that causes the plugins to be loaded.

This requires some more thought.

@fxlb
Copy link
Member

fxlb commented Nov 14, 2021

Rebased.

@infrastation
Copy link
Member

If the only functions implemented by a plugin would be a custom read function and a custom write function, would it be simpler to use popen() instead? For example, tar allows a number of different compression filters, each of which runs as a separate process.

@infrastation
Copy link
Member

the-tcpdump-group/tcpdump#458 seems to implement a similar idea, although the code seems a bit more complicated than I thought and uses fork() + execvp() instead of popen().

@infrastation
Copy link
Member

the-tcpdump-group/tcpdump#935 is yet another alternative implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants