Created attachment 130238 [details] [review]
set pcap hdr snaplen to a valid value, ie below 262144
As reported in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=846241
with debian or upstream libpcap 0.8 we get the error:
hGetContents: invalid argument (invalid byte sequence)
reading bustle-pcap generated dumps.
It turns out that snaplen supported by libpcap is 262144 at most while c-sources/pcap-monitor.c initable_init set it to "1 << 27" ie 134217728.
Setting this value to 65535 instead fixes the bustle-pcap newly generated files
and let bustle load them.
I had to add trace in libghc-pcap-dev then rebuild bustle to get the libpcap error :
invalid file capture length 134217728, bigger than maximum of 262144
"1 << 27" isn't a number plucked out of thin air: it is the maximum length of a D-Bus message. Per <https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-messages>:
> The maximum length of a message, including header, header alignment padding, and body is 2 to the 27th power or 134217728 (128 MiB).
It's annoying that libpcap does not expose its "somewhat arbitrary" limit in the API. https://github.com/the-tcpdump-group/libpcap/blob/503e21ed3be32079daeef6f9a7eee3544868f326/pcap-int.h#L87-L108
So a patch like the one you sent is necessary, but I guess Bustle will either have to fragment longer D-Bus messages across multiple pcap packets, or drop them.
And nobody bothered to inform libpcap developers that D-Bus messages can be bigger than the current maximum snaplen.
It's getting to the point that libpcap should probably, in order not to have people troll programs running on 32-bit systems by handing them a file with a huge snaplen, initially allocate a buffer of size MIN(MAXIMUM_SNAPSHOT_LENGTH, snapshot length in file) and grow the buffer as necessary.
That way, if somebody generates an Ethernet file with a snaplen of 2^31-1 (yes, I think there's at least one program that does that), you won't eat 4GB-1 of address space for the pcap_t for the file. It doesn't *prevent* a program from eating all the address space, but at least that'll only happen if it's necessary.
I'll look at that.
I'm working on making `bustle-pcap` (the tool, and the embedded copy in the GUI) replace the body of messages longer than 65535 bytes with a short string explaining that the body has been dropped. https://github.com/wjt/bustle/commit/aa52f3c269046cc01d8340789caa1ffec4e202e7 – apparently I can't push to fd.o today.
But… `dbus-monitor --pcap` will need a similar change.
(In reply to Will Thompson from comment #3)
> I'm working on making `bustle-pcap` (the tool, and the embedded copy in the
> GUI) replace the body of messages longer than 65535 bytes with a short
> string explaining that the body has been dropped.
I'm not sure whether even that is a general solution, because in principle there's nothing to stop the header from being almost arbitrarily long. The D-Bus Specification doesn't seem to have anything to say that the header can't be as long as the 12 byte fixed part (yyyyuu) plus the 64M maximum length specified for the array that follows, or possibly 16 bytes + 64M if we interpret the somewhat ambiguous spec wording as imposing a 64M limit on the elements of the array (after its length) rather than the array as a whole.
Unfortunately, the answer might have to be for monitors like bustle-pcap and dbus-monitor to fragment large D-Bus messages into multiple pcap packets. I think it would be reasonable for readers to assume that for "native" D-Bus captures (as opposed to TCP captures that happen to be a D-Bus stream), the first packet of each message has at least the 16 bytes that are needed to determine the length, and the beginning of each message starts a new packet even if the previous packet was undersized - that's certainly what I'd implement in dbus-monitor.
Most of the work here is to be done by the reader (Bustle, and secondarily other pcap consumers like Wireshark) rather than the writer, so I'll defer to whatever you want to implement.
If we can do the fragmentation at a round number (32768 or 65536 rather than 65535) then the number of fragments in a message will probably make more sense to a reader.
(In reply to Guy Harris from comment #2)
> And nobody bothered to inform libpcap developers that D-Bus messages can be
> bigger than the current maximum snaplen.
Sorry, the library API didn't have an obvious maximum, so it didn't occur to us that there was any arbitrary limit other than the (4GB - 1) implicit in the file format (which is much longer than D-Bus' own arbitrary limit of 128M).
I see that Guy has committed a change to libpcap to truncate individual packets which are longer than MAXIMUM_SNAPLEN (262144) rather than refusing to read the whole file. Great!
So my revised plan for Bustle is to back out my patch to truncate at capture time, ship this patch in the Flatpak (and encourage distros to ship it unless it's in a release?), and handle reading truncated messages semi-gracefully. (Unfortunately the Haskell D-Bus implementation I'm using doesn't expose unmarshalling just the header. I guess nor does GDBus.) I think this is better than mangling or fragmenting the messages at capture time.
Oh, even better, there's special handling to not truncated D-Bus messages. Thanks! https://github.com/the-tcpdump-group/libpcap/commit/1a6b088a88886eac782008f37a7219a32b86da45
Bustle 0.6.1 will no longer crash if reading the log with libpcap fails – it'll display an error message.
I've created a Flatpak manifest that bundles Bustle with a recent-enough libpcap snapshot to support growing the buffer size up to the D-Bus protocol's limit. With luck it'll be on Flathub soon: https://github.com/flathub/flathub/pull/61
So I consider this bug fixed from Bustle's side. Distros may want to backport the libpcap patches linked above until a new libpcap release is made.