Raybellis gzip v2 #982

mcr · 2020-12-19T19:03:29Z

This is a rebase of Ray Bellis' gzip interface for pcap.

gvanem · 2020-12-19T20:00:43Z

pcap-ioplugin.c

+                fp = stdin;
+                if (stdin == NULL) {
+                        snprintf(errbuf, PCAP_ERRBUF_SIZE,
+                                 "The standard input is not open");


This is not a good test for a safe use of stdin since it could be a fixed address (like on djgpp).
Perhaps if (!stdin || ferror(stdin)) is better.

Is djgpp still a build target that matters? (I'm seriously asking: I didn't think so).
I'm happy to do something different here, but I want to make sure that it's really better.
I don't understand what !stdin would do? That's the same as stdin != NULL, isn't it?

changed based upon suggestion.

Is djgpp still a build target that matters?

Not really. But there could be other exotic targets were #define stdin (&__internal_stdin).

On my Solaris 11 VM:

/usr/include/stdio.h includes

#include <iso/stdio_iso.h>

and /usr/include/iso/stdio_ios.h includes

#if defined(__STDC__) extern __FILE __iob[_NFILE]; #define stdin (&__iob[0]) #define stdout (&__iob[1]) #define stderr (&__iob[2]) #else extern __FILE _iob[_NFILE]; #define stdin (&_iob[0]) #define stdout (&_iob[1]) #define stderr (&_iob[2]) #endif /* __STDC__ */

That goes all the way back to System N, for some Roman-numeral version of N, so other commercial UN*Xes may do that, too.

(GNU libc doesn't, so few Linux distributions do that; as far as I know, Chris Torek's stdio code doesn't, either, so I don't think any of the BSD-favored UN*Xes, including macOS, do that.)

I don't understand what !stdin would do? That's the same as stdin != NULL, isn't it?

Yes.

gvanem · 2020-12-19T20:01:04Z

pcap-ioplugin.c

+
+	if (strcmp(fname, "-") == 0) {
+		fp = stdout;
+                if (stdout == NULL) {


Same issue here.

also updated.

mcr · 2020-12-19T21:39:24Z

configure

@@ -4643,7 +4658,7 @@ else
    We can't simply define LARGE_OFF_T to be 9223372036854775807,
    since some C++ compilers masquerading as C compilers
    incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))


@guyharris is this an error, or something we should be doing?
This seems to have jumped into this patch, and I don't know why.

This might just be a case of different versions of autoconf generating different code.

guyharris · 2020-12-25T11:22:55Z

testprogs/selpolltest.c

@@ -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;
+	int selectable_fd = -1;


This is presumably done to work around compilers that don't figure out that if neither doselect nor dopoll are set, selectable_fd isn't used.

It belongs in a separate change (which shouldn't require any review, so you might as well just commit it).

fixed on main in fbb5e9e

guyharris · 2020-12-25T11:26:19Z

sf-pcap.c

@@ -821,23 +805,19 @@ pcap_dump_open(pcap_t *p, const char *fname)
 		    "A null pointer was supplied as the file name");
 		return NULL;
 	}
-	if (fname[0] == '-' && fname[1] == '\0') {


If all plugins are supposed to treat "-" as meaning "write to the standard output", pcap_setup_dump() should probably do so as well, and report that a write error occurred to "(the) standard output" rather than to "-", as the code currently does.

guyharris · 2020-12-25T11:27:55Z

sf-pcap.c

@@ -794,6 +777,7 @@ pcap_setup_dump(pcap_t *p, int linktype, FILE *f, const char *fname)
 pcap_dumper_t *
 pcap_dump_open(pcap_t *p, const char *fname)
 {
+	const pcap_ioplugin_t *plugin = pcap_ioplugin_init(getenv("PCAP_IOPLUGIN_WRITE"));


Presumably your other pull request will eventually allow a program using libpcap to choose to use different I/O routines, so that, for example, a program could provide a command-line flag, or a GUI checkbox, or something such as that to do compressed output, rather than requiring the user to set an environment variable.

guyharris · 2020-12-25T11:32:11Z

The use of space indentation vs. tab indentation isn't consistent.

guyharris · 2020-12-25T11:33:09Z

@mcr: Speaking of I/O plugins, would that be something that could be used to do asynchronous I/O, especially when writing a capture file? I think you have some ideas about doing async I/O when writing.

guyharris · 2020-12-25T11:56:49Z

pcap-ioplugin.c

+#include "ftmacros.h"
+
+#ifdef _WIN32
+#include <pcap-stdinc.h>


That file no longer exists; to quote the message in the commit that removed it:

commit 787f37eeffdc3c37beeca8357d667ef35880c62b Author: Guy Harris <gharris@sonic.net> Date: Tue Sep 5 13:51:37 2017 -0700 Get rid of pcap-stdinc.h. On Windows, in each file, include whatever that particular file needs, just as we do on UN*X and MS-DOS.

guyharris · 2020-12-25T12:19:39Z

I presume there's no guarantee that, for an arbitrary plugin, there will be a FILE * corresponding to the input or output stream - or that, even if there is, it will contain the full state of the stream, given that the compression/decompression code might require its own state.

In addition, I'm not sure how control gets handed to the plugin when the sf-pcap.c code attempts to read from or write to the file - or how this handles reading from pcapng files at all (we don't yet support writing pcapng files, so that's not an issue, yet).

Thus, on the read side, we might want to:

add a flag to a struct pcap to indicate whether it's for a live capture or a savefile;
put some members of struct pcap into a union, with one element of the union being a structure for live captures and another one being a structure for savefiles;
put the rfile member into the union for savefiles, and change it to be a void *;
have pcap_file() return NULL for a live capture and return the rfile member, cast to FILE * for a savefile;
deprecate pcap_file(), as, in the general case, all we can guarantee is that it'll return NULL for a live capture and something non-NULL, but not guaranteed to be usable with standard I/O routines, for a savefile;
add pcap_is_savefile(), which returns 0 for live captures and 1 for savefiles (meaning "return the "this is a savefile" flag mentioned above;
have read plugins have a read routine as well as an open-for-reading routine, and write plugins have a write routine as well as an open-for-writing routine, with the read and write routines taking fread() and fwrite()-style arguments, but with the void * passed, rather than a FILE *, as the handle argument, and with the stdio read and write routines being tiny wrappers around fread() and fwrite();
have a pointer to the read routine, and a pointer to the handle returned by the open-for-reading routine, be members of the savefile structure mentioned above;
have the sf-pcap.c and sf-pcapng.c code call the read routine, handing it the pointer to the handle, whenever they nee to read data from the savefile.

guyharris · 2020-12-25T22:35:49Z

It looks as if his code depended on the standard I/O routines supporting the caller being able to supply I/O methods to use on the FILE *; that works with GNU libc and with BSD-flavored libc's, but not with all UN*Xes and not on Windows.

I guess we could do step 1, using a FILE *, and then later redo it with a more general mechanism that allows arbitrary handles to be associated with the pcap_t; that would allow, for example, an asynchronous I/O backend to be provided, but it'd mean that existing plugins would have to be changed.

guyharris · 2020-12-25T22:36:09Z

This also needs CMake changes and a cmakeconfig.h.in file.

guyharris · 2020-12-26T10:04:42Z

Thus, on the read side, we might want to:

Unfortunately, there's no data structure on the write side, so we're stuck, for now, with relying on the standard I/O library allowing write routines to be added, unless we make a pcap_dumper_t not be a FILE *, which might break some code using libpcap that "knows" that it's a FILE *.

We should provide new writing routines that don't work that way.

mcr · 2020-12-29T22:13:27Z

@mcr: Speaking of I/O plugins, would that be something that could be used to do asynchronous I/O, especially when writing a capture file? I think you have some ideas about doing async I/O when writing.

Maybe. But, as currently structured in libpcap, it would require making a copy of the buffer.
I have some ideas and some code from a failed project earlier in 2020 on doing it without a copy.
It will only work if pcap asks the kernel for rather big ring size, but on machines that want to capture everything, that seems a reasonable thing to do.

mcr · 2020-12-29T22:34:33Z

example code at https://github.com/raybellis/libpcap-gzip and
https://github.com/DNS-OARC/dnscap/blob/develop/src/dumper.c

…e-tcpdump-group#982 (cherry picked from commit fbb5e9e)

infrastation · 2021-11-14T16:46:18Z

These changes are a strict subset of pull request #914.

…e-tcpdump-group#982

mcr added 3 commits December 19, 2020 14:01

IO plugins for libpcap

675dbfe

add comments about where this pointer to function usually points

4a1344f

allow for ioplugin to default to off, enabling with --enable-ioplugin

2e7e32f

mcr requested a review from guyharris December 19, 2020 19:06

gvanem reviewed Dec 19, 2020

View reviewed changes

mcr commented Dec 19, 2020

View reviewed changes

guyharris reviewed Dec 25, 2020

View reviewed changes

mcr added a commit that referenced this pull request Dec 29, 2020

make some compilers be quiet about un-initialized variable, as per #982

fbb5e9e

mcr mentioned this pull request Dec 29, 2020

some suggested structuring for struct pcap for libpcap 2.0 #984

Open

fxlb pushed a commit to fxlb/libpcap that referenced this pull request Jan 1, 2021

make some compilers be quiet about un-initialized variable, as per th…

372c682

…e-tcpdump-group#982 (cherry picked from commit fbb5e9e)

infrastation closed this Nov 14, 2021

infrastation deleted the raybellis-gzip-v2 branch November 14, 2021 16:42

tenarchits pushed a commit to tenarchits/libpcap that referenced this pull request Mar 4, 2023

make some compilers be quiet about un-initialized variable, as per th…

54c4bda

…e-tcpdump-group#982

guyharris mentioned this pull request Jun 11, 2024

libpcap should fully support pcapng files #1321

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raybellis gzip v2 #982

Raybellis gzip v2 #982

mcr commented Dec 19, 2020

gvanem Dec 19, 2020

mcr Dec 19, 2020

mcr Dec 19, 2020

gvanem Dec 20, 2020

guyharris Dec 25, 2020

guyharris Dec 25, 2020

gvanem Dec 19, 2020

mcr Dec 19, 2020

mcr Dec 19, 2020

guyharris Dec 25, 2020

guyharris Dec 25, 2020

mcr Dec 29, 2020

guyharris Dec 25, 2020

guyharris Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 26, 2020

mcr commented Dec 29, 2020

mcr commented Dec 29, 2020

infrastation commented Nov 14, 2021

Raybellis gzip v2 #982

Raybellis gzip v2 #982

Conversation

mcr commented Dec 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

Choose a reason for hiding this comment

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 25, 2020

guyharris commented Dec 26, 2020

mcr commented Dec 29, 2020

mcr commented Dec 29, 2020

infrastation commented Nov 14, 2021