[João Valverde <j@xxxxxx>]

On 14/02/22 15:34, João Valverde wrote:
> Hi Jaap,
>
> If I understand correctly I think the numbers are correct by design.
>
> When viewing packet details the analysis is almost always on the 
> protocol header. In this case that's what the size represents and 
> that's what  I would expect.
>
> I don't typically use Protocol Hierarchy statistics but I think 
> counting total protocol size (header + payload) is a lot more 
> interesting and useful. That matches my understanding of PDU size, 
> which is presumably what I'm trying to look at with this statistic.
>
> I think there is still a bug lurking with fixed-size headers vs 
> variable size headers that needs to be fixed, to match the current 
> behavior.
Fixed-length headers are under-counting (as I understand the statistic 
described in the User's Guide) but the fact that variable length 
protocols work seems accidental. I think they are over-counting if there 
is a trailer present.

And also, it's not really correct to include IP inside ICMP as IP bytes, 
but that's another issue entirely.

> On 13/02/22 23:18, Jaap Keuter wrote:
> > This discussion was brought on by issue 17877 titled “Non-visible 
> > photo items cannot change length after construction”. In there it is 
> > correctly stated that calls to set proto_item length (either 
> > proto_item_set_len() or proto_item_set_end()) are not effective when 
> > proto_items are not visible. When looking at these functions it can 
> > be seen that these are part of the group of functions which are 
> > optimised for faking proto_items when not visible.
> >
> > Without going into the faking details (see the macro 
> > TRY_TO_FAKE_THIS_ITEM_OR_FREE in epan/proto.c for that) it comes down 
> > to just short cutting the proto_item creation process by returning 
> > the tree intended to attach the newly to be constructed proto_item 
> > to. In effect these all return the root node of the dissection tree.
> >
> > A special case exception is put in place for proto_items of type 
> > FT_PROTOCOL. EPAN can be setup so that proto_items of this type in 
> > fact are allowed to be created, even if they otherwise would have 
> > been faked. This feature is used for protocol level statistics. The 
> > protocol level statistics ’tally the numbers’ by running the 
> > dissection, with a hidden tree, but with the special case exception 
> > set. This results in a very compact dissection tree, consisting of 
> > the root node and the proto_items of type FT_PROTOCOL. The length of 
> > these items is then used to determine the numbers to add to the 
> > various protocol layers in the statistics tree.
> >
> > This is where the ambiguity comes in. Some protocols claim just there 
> > share of the octets in the frame (discussing Ethernet packet 
> > dissection here, to keep it simple). Others create their proto_item 
> > until the end of the TVB handed to them (usually to the end of the 
> > frame), and adjust the length after dissection of their fields have 
> > taken place and the variable number of bytes in the protocol layer 
> > has been determined. However, the functions to set these lengths 
> > don’t work when faking items is in effect.
> >
> > As a result these protocols take up way more of the frame in the 
> > statistics than they in fact do. Overall more the 100% of the frame 
> > is allotted to the protocols contained in them. The User's Guide goes 
> > into this fact with the explanation that these protocol 'contain’ 
> > their payload, so that is why the payload is added to the protocol. 
> > That is one interpretation, but not really consistent because fixed 
> > size dissectors, which create their proto_items of type FT_PROTOCOL 
> > with fixed size, do not exhibit this behaviour.
> >
> > The simplest step to take would be to allow the functions 
> > proto_item_set_len() and proto_item_set_end() to operate on 
> > proto_items of type FT_PROTOCOL if the afore mentioned special case 
> > exception was in effect. However, since faking of other types of 
> > proto_items is still in effect, all these other proto_items are now 
> > using the proto_items of type FT_PROTOCOL as proto_item, rather than 
> > the root node. This means that code setting the length of a field, is 
> > now also no longer blocked, and in fact setting the length of the 
> > proto_item of type FT_PROTOCOL rather than his own (which is faked).
> >
> > A simple experiment with the file (code_mac_tagged.pcap) attached to 
> > issue 17877 makes this clear. Changing proto_items_set_len() to allow 
> > proto_items with type FT_PROTOCOL to set their length if the special 
> > case exception is set, shows a protocol hierarchy statistics page 
> > with all protocols matching their length in the dissection details 
> > pane. Except for COSE, the dissection details say 309 while the 
> > statistics say 246. This last length is the length set for the final 
> > part of the PDU, which ends up becoming the length of the protocol in 
> > the protocol hierarchy statistics.
> >
> > The question now becomes how to proceed with this. Faking proto_items 
> > makes legitimate calls to set the length of proto_items of type 
> > FT_PROTOCOL indistinguishable from those calls meant for fields 
> > within those protocols. Another approach of faking proto_items by 
> > always returning the root node instead of the tree creates it’s own 
> > set of side effects. Now all ’non-protocol’ proto_items are processed 
> > for statistics too, and exposing things like expert info in the 
> > statistics also. This defeats the purpose of faking proto_items in 
> > the first place.
> >
> > A ‘quick and dirty’ solution is to run dissection with a visible 
> > tree. This gives the desired results, but at the cost of doing a lot 
> > more dissection work than strictly necessary, voiding the whole 
> > purpose of the special case exception in not faking proto_items with 
> > type FT_PROTOCOL.
> >
> > I’m not seeing a simple way out of this. Do we want to modify the 
> > statistics in this way is the fist question to answer. They will be 
> > different than in 3.6, 3.4 and earlier. This could be the right time 
> > to do it though. Then the question becomes how to achieve this 
> > without taking a significant performance hit by sidestepping the 
> > faking of proto_items.
> >
> > Thanks,
> > Jaap
> >
> > (not rereading this draft, it’s way too late for that. Sorry for any 
> > mistyping or confusing statements)
> >
> >
> >
> > ___________________________________________________________________________ 
> >
> > Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> > mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>
> ___________________________________________________________________________ 
>
> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe