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

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

So just to be clear: should I, or shouldn't I, implement the solution I
suggest below and submit a patch?

-----Original Message-----
From: Shaked, Nitzan 
Sent: Thursday, March 19, 2009 2:57 PM
To: tcpdump-workers () lists tcpdump org
Subject: RE: [tcpdump-workers] Bug in libpcap: savefile.c /
get_selectable_fd()

Cool. I appreciate the comments, and agree to pretty much most of them.

What's next? Realistically speaking, should I hold my breath for those
changes? (Should I implement them and submit a patch?)

One comment: while strictly speaking what you write below about the
changes having to be done even now if I want to read from a pipe with
select(), in reality calling setvbuf() on the FILE* works great for me.
That's because I read from a pipe, with tcpdump -w writing to it. So if
there is SOMETHING in the pipe, even if it's only half a header or
something like that, I'm guaranteed that the rest will follow shortly (I
run tcpdump -U -w). Since the fread()'s in sf_next_packet() are
blocking, I don't have to change the code AND it returns quickly enough.

Thanks again,
Nitzan

-----Original Message-----
From: tcpdump-workers-owner () lists tcpdump org
[mailto:tcpdump-workers-owner () lists tcpdump org] On Behalf Of Guy Harris
Sent: Thursday, March 19, 2009 12:34 PM
To: tcpdump-workers () lists tcpdump org
Subject: Re: [tcpdump-workers] Bug in libpcap: savefile.c /
get_selectable_fd()


On Mar 19, 2009, at 2:39 AM, Shaked, Nitzan wrote:

> 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",

...and it would even need that if you got rid of buffering in the  
savefile code, as, when select() says the descriptor is readable,  
there's no guarantee that what will be readable from the pipe will be  
a collection of one or more full records with no fraction of a record  
at the end.

> 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, ... );

(fcntl() doesn't work on FILE *'s, it works on file descriptors, so  
you'd have to call fileno().)

>       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.

No, you'd call pcap_file() and then call fileno() and call fcntl() on  
that.

Or, given that, as indicated above, in order to use select() when  
reading from a pipe we'd have to change the savefile code *anyway*, we  
could make pcap_setnonblock() to work on savefiles.
        
> 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().

Everybody *who needs to use select()* would do so.  *NOT* all  
applications using pcap_open_offline() need to use select(); tcpdump,  
for example, doesn't.

> 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).

(I'll wait until somebody has a version of libpcap() running on  
TOPS-10 or TOPS-20 or Unix-on-Univac-mainframes or... before we worry  
about that.  Programs using libpcap/WinPcap would have plenty of  
portability issues on those platforms over and above any portability  
issues libpcap has; all the current platforms on which libpcap runs  
have 8-bit bytes.)

> 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.

If we're in blocking mode, the same could be true, as indicated  
above.  If you really want to be able to use libpcap to read from a  
pipe, without blocking indefinitely on the pipe, with the aid of  
select(), the code *has* to be able to deal with that, even if it does  
only one-byte read() calls, as select() might tell you that there's  
only one byte to read.

> 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:

It can do whatever's necessary, as it's static.

> 4.2) pcap_offline_read() should return:

        ...

>       OFFLINE_READ_BREAK_LOOP (value: -3), if we were told to break
> out of the loop

No, that breaks binary compatibility.  It'd have to continue to return  
-2 when told to break out of the loop.
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.