search

rusticata / pcap-parser

PCAP/PCAPNG file format parser written in pure Rust. Fast, zero-copy, safe.
Other
103 stars 24 forks source link

Add PcapError::UnexpectedEof error type #28

Closed iczero closed 7 months ago

iczero commented 1 year ago

pcap_parser does not currently handle a truncated packet at the end of a pcap file. It will return PcapError::Incomplete, which with the given example, will cause an infinite loop. Add an UnexpectedEof variant which will be returned when the reader is EOF but nom still wants more data.

Additionally, expose how many more bytes are wanted in PcapError::Incomplete.

chifflier commented 7 months ago

Hi, Thank you for your contribution The idea in returning Incomplete is to support streaming parsers, and be able to request more data on demand. I need to run more tests before merging this, and in particular check that all cases (incomplete read, reader exhausted etc.) are still working

iczero commented 7 months ago

The UnexpectedEof variant should only be raised if nom wants more bytes but a previous read completed successfully with zero bytes read, otherwise, Incomplete(n) (where n is the number of bytes wanted, or 0 if unknown) will be returned as usual. read should only return 0 bytes on EOF, however if a file is being appended to, it may return more bytes later. I believe it should be possible to call refill if UnexpectedEof is returned if it is expected that the file is being appended to. I have successfully tested streaming parsers with tcpdump -w - | whatever.

It appears that I have forgotten to update the readme.

chifflier commented 7 months ago

The intention looks good to me. The goal is to be able to clearly expose the difference between EOF as returned by underlying reader, and missing bytes. However, for some reason that seems unexpectedly difficult, and problems like #29 are still present with these commits. I'm investigating why.

iczero commented 7 months ago

Regarding #29, I think the issue there is that the buffer size is smaller than the maximum chunk size, which results in refill() doing no forward progress. This patchset won't directly fix that problem, but the user should be able to check the additional bytes wanted from Incomplete and either resize the buffer or fail if it exceeds the current buffer size.

chifflier commented 7 months ago

Regarding #29, I think the issue there is that the buffer size is smaller than the maximum chunk size, which results in refill() doing no forward progress. This patchset won't directly fix that problem, but the user should be able to check the additional bytes wanted from Incomplete and either resize the buffer or fail if it exceeds the current buffer size.

yes, my understanding is the same. Since the buffer is full, .refill() will succeed but do nothing, and the call to .next() will still return incomplete.

I think the only way to detect it would be to test if buffer_available + missing > buffer_capacity, provided that we know missing (done in your PR) and the parser does not return Unknown.

My interrogation is more to decide if this should be the caller responsibility to check this, of if we should do that in .next() (for ex. at this point). There would be a small performance cost (which I think is acceptable) on Incomplete.

chifflier commented 7 months ago

Merged, thanks! I'll move the discussion on infinite loop to #29