Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packet-buffered mode (-U) doesn't affect packet printing #1213

Open
GD1200 opened this issue Aug 31, 2024 · 10 comments
Open

Packet-buffered mode (-U) doesn't affect packet printing #1213

GD1200 opened this issue Aug 31, 2024 · 10 comments

Comments

@GD1200
Copy link

GD1200 commented Aug 31, 2024

I noticed that the packet-buffered mode (-U) has no effect when STDOUT is not a terminal and raw packets recording (-w) is not used. If my STDOUT is tee, the terminal gets updated only when tcpdump's output buffer is full. In the example below, 10 ICMP packets had already been captured but the output buffer was not full yet, so nothing was printed to the terminal:

$ sudo tcpdump -Uni wlan0 icmp | tee /dev/null
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on wlan0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
^C10 packets captured
10 packets received by filter
0 packets dropped by kernel

I understand that -U is primarily used in combination with -w, but the manual says that it can also be used without -w:

   -U
   --packet-buffered
          If the -w option is not specified, or if it is specified but the --print  flag
          is  also  specified, make the printed packet output ‘‘packet-buffered''; i.e.,
          as the description of the contents of each packet is printed, it will be writ‐
          ten to the standard output, rather than, when not writing to a terminal, being
          written only when the output buffer fills.

It was tested against the latest stable tcpdump and libpcap versions:

$ tcpdump --version
tcpdump version 4.99.4
libpcap version 1.10.4 (with TPACKET_V3)
OpenSSL 3.3.1 4 Jun 2024
@infrastation
Copy link
Member

Please re-test using tcpdump 4.99.5.

@GD1200
Copy link
Author

GD1200 commented Aug 31, 2024

@infrastation, I have just tried both 4.99.5 and today's master but see no difference:

# /usr/local/tcpdump-git/bin/tcpdump -Uni ens3 icmp | tee /dev/null
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on ens3, link-type EN10MB (Ethernet), snapshot length 262144 bytes
^C10 packets captured                          
10 packets received by filter
0 packets dropped by kernel

# /usr/local/tcpdump-git/bin/tcpdump --version                                                                                                                                
tcpdump version 5.0.0-PRE-GIT                                                                                                                                                                 
libpcap version 1.10.3 (with TPACKET_V3)                                                                                                                                                      
64-bit build, 64-bit time_t


# /usr/local/tcpdump-4.99.5/bin/tcpdump -Uni ens3 icmp | tee /dev/null
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on ens3, link-type EN10MB (Ethernet), snapshot length 262144 bytes
^C10 packets captured
10 packets received by filter
0 packets dropped by kernel

# /usr/local/tcpdump-4.99.5/bin/tcpdump --version
tcpdump version 4.99.5
libpcap version 1.10.3 (with TPACKET_V3)
64-bit build, 64-bit time_t

@sbakker
Copy link

sbakker commented Sep 1, 2024

The code in master is fairly simple to trace regarding this; it's all contained in tcpdump.c. The -U option sets the Uflag variable (static int) on line 1809. It is then checked on lines 2432, 2999, and 3022; the only effect it has is to cause a call to pcap_dump_flush().

So, either the doc is wrong (i.e. -U has no meaning outside of -w), or an additional fflush(stdout) should be called at the above mentioned locations.

@guyharris
Copy link
Member

I understand that -U is primarily used in combination with -w, but the manual says that it can also be used without -w:

That was a mistake made by the author of this commit:

commit 56a6db2e51fbfc67c77962d2e336f58e704a3f01
Author: Guy Harris <[email protected]>
Date:   Sat Mar 10 15:23:50 2012 -0800

    Give more information on -l, and note that -U works without -w.
    
    Note that -l, on Windows (i.e., in WinDump), is unbuffered, not
    line-buffered, and describe -U as an alternative (that doesn't have that
    problem on Windows).
    
    Note that -U does packet-buffering without -w.
    
    Fix up the formatting of the example command lines with -l.

The mistake is the "Note that -U does packet-buffering without -w." part; sadly, it doesn't.

So:

So, either the doc is wrong (i.e. -U has no meaning outside of -w)

is correct. However:

or an additional fflush(stdout) should be called at the above mentioned locations.

the "or" there is not correct; some form of packet-buffering should be provided, given that -l, whilst it provides line buffering on UN*Xes, does not provide it on Windows - or, at least, on Windows if the Visual Studio C library is used - for reasons known only to Microsoft. Instead, it makes the standard output completely unbuffered, which is... less than ideal.

I'm not sure what, if anything, would break if -l were made to do packet-buffering.

-U was introduced in

commit b8e2c3beadd349cef70e6246536feb84d203c826
Author: guy <guy>
Date:   Sun Dec 22 00:15:26 2002 +0000

    From Andrew Brown <[email protected]>: add a "-U" flag, which causes
    the output stream for "-w" to be flushed after each packet is dumped.
    
    Add checks for "pcap_dump_flush()", and only enable the "-U" flag if
    it's present.  Clean up the handling of the "getopt()" argument and the
    usage message to get rid of the pile of #ifdefs.
    
    Add documentation for the "-L" and "-y" flags.
    
    Tweak the description of "-r" to properly format "-w" in the text.

Presumably that was a patch provided by Andrew Brown, but I can't find it anywhere, so I don't know whether 1) it originally made writing to the capture file completely unbuffered, hence the -U, and I changed it to flush after each packet is dumped, or 2) it flushed after each packet is dumped and -U wasn't chosen for full accuracy.

This also raises the issue of, if you use --print and -w so that it writes packets to a capture file and prints dissected packets to the standard output, whether -U should always affect both.

But, for starters, the standard output should be flushed after printing packets if -U is specified.

@guyharris guyharris changed the title Packet-buffered mode (-U) is ignored when STDOUT is not a terminal and -w is not used Packet-buffered mode (-U) doesn't affect packet printing Sep 1, 2024
@sbakker
Copy link

sbakker commented Sep 2, 2024

My suggestion was not to change the functioning of -l, but rather to add a fflush() call when Uflag is set:

        if (Uflag) {
            pcap_dump_flush(pdd);
            fflush(stdout);
        }

That way, -U will do what is currently advertised. If -l is also given, then this will have no effect on output to stdout (i.e. -l "overrides" -U), otherwise it provides per-packet ouput to stdout, even in the face of redirected/piped output.

@guyharris
Copy link
Member

My suggestion was not to change the functioning of -l

The problem the original filer wants to solve isn't "-U doesn't affect packet printing", it's "they want a mode in which dissected packets are written out as soon as they arrive, even when not written to a terminal". -l is such a mode, but it isn't the ideal mode, as it's not line-buffered on Windows, it's unbuffered on Windows.

The TShark program in the Wireshark suite solved that problem by making -l packet-buffered rather than line-buffered, which also has the advantage on UN*X of being a bit more efficient if more than one line is written per packet, as it doesn't do a separate write() for every line.

I don't know what would break if we were to do the same for tcpdump. If nothing would break, or if the breakage is trivial, we might want to do that; with -v, tcpdump can print more than one line per packet, and there might be delays if, for example, lines after the first include IP addresses for which name resolution is attempted, so that being packet-buffered might cause earlier lines not to be printed until name resolution for later lines finishes.

A separate problem is that I mistakenly changed the tcpdump man page to document that -U also causes packet buffering for dissected packet output when it does not, in fact, do so. That problem could be fixed either by making it do so or by changing the documentation not to say it does so.

The first of those solutions provides packet buffering of dissected packet output without changing the behavior of -l, but means there's no option to provide packet buffering of raw output to a capture file without also providing packet buffering of dissected packet output. It may be that, in most but not all cases, somebody who is doing both capture to a capture file and printing dissected packets to something other than a terminal (such as a file or a pipe), and wants packet-buffered output to the capture file, would also want packet-buffered output of the dissected packets as well, in which case that -U behavior would be a feature, not a bug.

The second of those solutions fixes the disparity between the documentation and the code, without changing the behavior of -l, but also without providing a packet-buffered form of output for dissected packets.

@sbakker
Copy link

sbakker commented Sep 3, 2024

Thanks for the clarification. Yes, my proposal was to do what you outline in the last two paragraphs. From my (admittedly) limited view of the world it would work, but it would indeed break the case where a user wants stdout to be packet buffered, with pcap dump (-w) fully buffered. It's a conundrum...

@infrastation
Copy link
Member

infrastation commented Sep 3, 2024

So, maybe it would be best to fix the man page first to document the currently implemented behaviour, and then when/if the behaviour changes, change the man page accordingly?

@GD1200
Copy link
Author

GD1200 commented Sep 3, 2024

The problem the original filer wants to solve isn't "-U doesn't affect packet printing", it's "they want a mode in which dissected packets are written out as soon as they arrive, even when not written to a terminal". -l is such a mode, but it isn't the ideal mode, as it's not line-buffered on Windows, it's unbuffered on Windows.

-l has been working perfectly well on our Linux systems; it was that I simply noticed that -U does not works at all when I was looking for a way to disable fully-buffered standard output writing.

but it would indeed break the case where a user wants stdout to be packet buffered, with pcap dump (-w) fully buffered.

And I see that the opposite would be true as well: lack of options to write raw packets in the packet-buffered way while keeping the standard output to non-tty destinations fully-buffered.

So, maybe it would be best to fix the man page first to document the currently implemented behaviour, and then when/if the behaviour changes, change the man page accordingly?

I also wanted to propose this as a first step.

The first of those solutions provides packet buffering of dissected packet output without changing the behavior of -l, but means there's no option to provide packet buffering of raw output to a capture file without also providing packet buffering of dissected packet output.

I guess it can get too confusing if another flag is added for packet-buffered standard output only?

@sbakker
Copy link

sbakker commented Sep 4, 2024

I guess it can get too confusing if another flag is added for packet-buffered standard output only?

In that case, I'd argue for an option with an argument: --print-flush={none|line|packet}

Or just, you know, fix the man page and live with -l. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants