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

[Guy Harris <guy () alum mit edu>]

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.