After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 795284 - pcapparse: give error on fragmented IP packets
pcapparse: give error on fragmented IP packets
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-15 22:02 UTC by Antonio Ospite
Modified: 2018-04-16 08:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pcapparse: add some comments about the pcap format headers (2.35 KB, patch)
2018-04-15 22:04 UTC, Antonio Ospite
committed Details | Review
pcapparse: bail out in case of fragmented packets (2.15 KB, patch)
2018-04-15 22:04 UTC, Antonio Ospite
committed Details | Review

Description Antonio Ospite 2018-04-15 22:02:22 UTC
Hi,

I was testing pcapparse on some capture of a Panasonic GH3 camera (the video data is some MJPEG over UDP variant BTW):
wget http://www.personal-view.com/talks/uploads/FileUpload/03/24c6c8d18eb6ca40775e1438464147.jpg -O LumixLink_captureStill.pcap

The Wireshark filter to see the interesting data is: "udp.srcport == 65416"

I didn't understand why, when specifying the src-port, no data was getting downstream. After adding some printouts and looking again at the wireshark logs I finally figured out it was because pcapparse cannot parse fragmented IP packets.

So what about adding an explicit check for this and warn the user with an error?

I will propose a patch for that, along with some comments which may help readers new to pcapparse code.

Ciao,
   Antonio
Comment 1 Antonio Ospite 2018-04-15 22:04:03 UTC
Created attachment 370969 [details] [review]
pcapparse: add some comments about the pcap format headers

Since the code is full of magic add at least some guidance for newbies.
Comment 2 Antonio Ospite 2018-04-15 22:04:09 UTC
Created attachment 370970 [details] [review]
pcapparse: bail out in case of fragmented packets

pcapparse cannot parse fragmented IP packets correctly, in particular it
will get confused when trying to parsing fragments as standalone frames
in two ways:

  1. the first fragment will have the packet length greater than the
     frame size and will always be discarded;

  2. fragments with non-zero offsets will be interpreted as full packets
     and the first part of their raw payload data will be parsed as the
     transport protocol header, resulting in bogus values for addresses
     and ports, thus evading the properties filtering on those values.

This can make it difficult for users to see why the data does not get
downstream.

So be more explicit and just bail out when fragmented packets are
encountered.
Comment 3 Sebastian Dröge (slomo) 2018-04-16 06:51:37 UTC
Could we also try to merge fragmented packets again and only drop them if not all fragments are available, i.e. what ever IP stack would do?
Comment 4 Antonio Ospite 2018-04-16 07:50:15 UTC
In the particular case of LumixLink_captureStill.pcap I managed to come up with a dumb mechanism to get all the fragments downstream.

However in general fragment reassembling is not super trivial, fragments can arrive out of order, and AFAIU fragments from different packets can be interleaved, so some state tracking with supplementary data structures is needed. And to detect incomplete packets some timeout mechanism is needed. See https://tools.ietf.org/html/rfc815

pcapparse seems to parse things as they come with no state tracking whatsoever so some refactoring seems to be needed, unless I am missing something.

Not something I can tackle right now, so I went for the at-least-admit-your-limits way.

Ciao,
   Antonio
Comment 5 Sebastian Dröge (slomo) 2018-04-16 08:04:25 UTC
Makes sense, thanks
Comment 6 Sebastian Dröge (slomo) 2018-04-16 08:22:38 UTC
Attachment 370969 [details] pushed as a4df513 - pcapparse: add some comments about the pcap format headers
Attachment 370970 [details] pushed as 53d7a12 - pcapparse: bail out in case of fragmented packets