Re: Bug in libpcap: savefile.c / get_selectable_fd()

["Shaked, Nitzan" <nshaked () paypal com>]

Hello again

Thanks for the reply. With your permission, I'd like to keep this up a
little bit (I am actually quoting not in the original order, to cover
the "easy" part first):

> If that means that you can't tell the difference between "end of file

> on the pipe", "no more packets available right now", and "an error  
> occurred while reading from the pipe", as might be the case, file a  
> bug on that.

Actually, it *will* mean that. pcap_next_ex() calls pcap_offline_read()
(in the case of a savefile), which in turns calls sf_next_packet(),
which does the fread().

If we introduce the notion of non-blocking, then two things happen:

1) The translation of return values from sf_next_packet() and from
pcap_offline_read() cannot, indeed, distinguish between EOF / IO error /
truncated packet, and needs to be somewhat changed.
2) I believe the current code doesn't have the notion of "non-blocking",
which it doesn't have the notion of "I read only part of what was
request, maybe half a packet, or half a header", so there will have to
be a new return value just for that. See below for a more elaborate
suggestion.

> I think you should put the pipe in non-blocking mode and, when  
> select() says the descriptor is readable, keep calling pcap_next()  
> until you get an error or EOF indication.

This seems like a good idea, and I'd love to actually do it. However,
there are currently several issues with it. I hope I am not overstepping
my boundaries here by listing my thoughts:

1) Encapsulation. From your suggestion to put the fd in non-blocking
mode, I understand you mean to actually call fcntl() myself (not from
inside libpcap, since pcap_setnonblock() does nothing on savefiles).
This means a flow which goes something like this:

if ( is_tty(filename) || is_pipe(filename) || is_socket(filename) ... )
{
        fp = fopen( ... );
        fcntl( fp, ... );
        pcap_fopen_offline( fp, ... );
}

... I can't use pcap_open_offline(), since it will do its own fopen()
and I'll lose the non-blocking effect. So in effect pcap_open_offline()
becomes redundant. Everybody will have to know about the special
non-blocking thing, do the logic I just mentioned above, and call
pcap_fopen_offline().

2) Bug (?) in the current code. In savefile.c, in pcap_fopen_offline(),
there's an fread() to read the savefile's header. The fread goes
something like "amt_read = fread( &hdr, 1, sizeof(hfr), fp )", and then
asking "if ( amt_read != sizeof(hdr) )". So we're asking to read
sizeof(hdr) chars (portability issue: not all platforms have 8bit
chars). If we're in non-blocking mode, and reading from a pipe, say, it
could be that there are only 4 bytes available right now, but there will
be more available later. The current code will give an error (truncated
header), because the amount returned will be 4. This raises 2 issues:

2.1) The code perhaps should do headers_read = fread( &hfr, sizeof(hdr),
1, fp ) and then ask whether headers_read==1 or not.
2.2) A more general issue, see below in #3.

3) The "non-blocking may return less than what you asked for" beast will
also manifest in all other freads. For example, the fread() to read a
packet header. Or the subsequent fread() to read the packet itself. In
those cases, maybe half the packet is available now and the other half
will be available in a subsequent call to fread(). This will happen
"all" the time (well, often enough), and so cannot result in an error.
Something like a state should be added here, saying "I am currently
reading a packet, and expecting 200 more bytes". If you get less than
200 bytes (say 50), return a new error code (something like
ERR_SF_AGAIN), and save those 50 bytes. Next time you are called
continue from there until you've read all 200 (or got a "real" error, or
EOF). Of course the same goes for reading the sf's header, and each
packet's header.

4) Regarding the SF_ERR_AGAIN I mention above, and regarding the
translation of return values from sf_next_packet() and
pcap_offline_read() I mention: once we introduce the notion of
non-blocking IO, it seems to me that we need to introduce new return
values all across:

4.1) sf_next_packet() should return:
        SF_OK (value: 1), in case everything went well
        SF_ERR (value: -1), in case of an IO error or a truncated
header/packet
        SF_EOF (value: 0), in case of EOF
        SF_AGAIN (value: -2), in case... well... that not enough data is
available right now, but might be available later.

4.2) pcap_offline_read() should return:
        "n" (n>0), in case everything went well, return the number of
packets read
        OFFLINE_READ_EOF (value: 0), in case sf_next_packet() returns
SF_EOF
        OFFLINE_READ_ERR (value: -1), in case sf_next_packet() returns
SF_ERR
        OFFLINE_READ_AGAIN (value: -2), in case sf_next_packet() returns
SF_AGAIN
        OFFLINE_READ_BREAK_LOOP (value: -3), if we were told to break
out of the loop

4.3) pcap_next_ex(), should return:
        PCAP_NEXT_OK (value: 1), in case everything went well
        PCAP_NEXT_AGAIN (value: 0), in case pcap_offline_read() returned
OFFLINE_READ_AGAIN (when reading from a savefile), or read_op() returned
0 (no packets before timeout, when reading from a live capture)
        PCAP_NEXT_ERR (value: -1), in case pcap_offline_read() returned
OFFLINE_READ_ERR (when reading from savefile), or read_op() returned -1
        PCAP_NEXT_EOF (value: -2), in case pcap_offline_read() returned
OFFLINE_READ_EOF

Note that a return value of 0 from pcap_next_ex(), according to my
suggestion, merges "no packets before timeout" when reading from live,
with "no data in non-blocking" when reading a savefile. I think they
have the same meaning to the application (basically: select() again and
call pcap_next_ex() the next time you can).

Anxiously awaiting comments,
Nitzan
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.