[David Castelow <david.castelow@xxxxxxxxx>]

Greetings,

I have been a long time user of WS and thought I'd like to attempt to contribute something back to the project.

I'd found an issue recently with what at the moment is an enhancement to a dissector I have in development but could impact others and have had a fix accepted for that (commit 9707d6ae), but it was a very small change.

Having done so I thought I'd take a bit more of a look at WS and whether I could add something else to the project.

I had a look at some bugs/issues and thought that enhanced handling of Out-of-Order (OOO) IP packets might be a helpful feature, and one that didn't seem like it would be too hard to get to grips with.

This was reported in issue 17392 - https://gitlab.com/wireshark/wireshark/-/issues/17392

I have implemented a couple of first pass fixes for this.

1) The first touches only the packet-ip.c code, and parses the data returned from reassembly.

2) The second modifies the reassembly code by adding an optional "expert_info" field passed in order that the enhancement is generic (though I've only implemented it for IPv4).

Both work to my satisfaction for the one example provided by garrymar (https://gitlab.com/garrymar) but this second did involve far more plumbing in than I'd hoped, as all 140+ modules that use reassemble functions require changes to the initialisation of the fragment_items list.  Fortunately I could automate the changes to a considerable extent.

I think option (2) will involve fewer computations as it's only called for the packet containing the reassembly, and as part of an existing loop.

The big limitation I can see to both approaches is that the OOO detection only works if the packet is successfully re-assembled.

I can see ways to add the OOO functionality even when packets are not successfully reassembled, but it would appear to require some fairly heavy re-working of some code in reassemble.c that currently has no visibility of the necessary structures (the proto_tree and packet_info).  I'd not do that unless there was some "buy-in" to the concept.

So my questions are

1) Is it wanted? (conversely "did I waste a day or two on this?")

2) Because the second option is implemented in a helper module (reassemble.[ch]) it does mean it will be easy to add to any/all protocols that support re-assembly, but is this the best way to achieve the goal? 

3) Does this feature need to be controlled (e.g. associated with a protocol preferences item)?  At the moment (and for IPv4 only) it is always present.

4) In the IPv4 I've used info level "warn" as that seems to be the appropriate level for this sort of issue.  Does this seem appropriate?

5) Are there better ways to pass in the information to the reassemble functions?

6) Code missing the necessary changes to the documentation to describe usage - I would add but little or no point if not being incorporated.

The code is available for review at my gitlab page, but given the large number of files impacted by my solution and my being very much a newbie I thought I'd raise the changes here before raising a PR/comments in the report.

Option 1) is on branch david/ooo-ip-only, option 2) on david/ooo-generic, both on david/ooo-both-methods

git@xxxxxxxxxx:daveysprockett/wireshark.git

In the process I noticed a handful of files in epan/dissectors/ that in the header claim "Do not modify this file. Changes will be overwritten." and for which there are instructions to re-create, but despite this they appear to be tracked in git and for one of them at least I was unable to re-create the file from the template in asn1/x/ - some of the item names differed.  I'd appreciate insight on how that needs to be done: I did modify the template files to match the new usage (this was seen a few weeks back, not re-checked recently).

Regards