[João Valverde <joao.valverde@xxxxxxxxxxxxxxxxxx>]

On 22/06/21 03:35, John Thacker wrote:
> On Mon, Jun 21, 2021 at 9:28 PM João Valverde via Wireshark-dev 
> <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>> wrote:
>
>
>
>     On 22/06/21 01:26, John Thacker wrote:
>      > On Mon, Jun 21, 2021 at 2:21 PM João Valverde via Wireshark-dev
>      > <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>
>     <mailto:wireshark-dev@xxxxxxxxxxxxx
>     <mailto:wireshark-dev@xxxxxxxxxxxxx>>> wrote:
>      >
>      >
>      >
>      >     On 16/06/21 15:36, David Perry wrote:
>      >      > Sorry to drag up an old topic, but I've been thinking
>     about this:
>      >      >
>      >      >> Message: 5
>      >      >> Date: Sat, 29 May 2021 09:32:29 +0200
>      >      >> From: Anders Broman <a.broman58@xxxxxxxxx
>     <mailto:a.broman58@xxxxxxxxx>
>      >     <mailto:a.broman58@xxxxxxxxx <mailto:a.broman58@xxxxxxxxx>>>
>      >      >> To: Developer support list for Wireshark
>      >     <wireshark-dev@xxxxxxxxxxxxx
>     <mailto:wireshark-dev@xxxxxxxxxxxxx>
>     <mailto:wireshark-dev@xxxxxxxxxxxxx
>     <mailto:wireshark-dev@xxxxxxxxxxxxx>>>
>      >      >> Subject: Re: [Wireshark-dev] Calling a dissector: Type
>     for data
>      >      >>     parameter
>      >      >> Message-ID:
>      >      >>
>      >     
>          <CAOpyz=zDycm33PXUwtBCTew7gTTEcSLiJ-f8SHW0L-863Q517A@xxxxxxxxxxxxxx <mailto:zDycm33PXUwtBCTew7gTTEcSLiJ-f8SHW0L-863Q517A@xxxxxxxxxxxxxx> <mailto:zDycm33PXUwtBCTew7gTTEcSLiJ-f8SHW0L-863Q517A@xxxxxxxxxxxxxx <mailto:zDycm33PXUwtBCTew7gTTEcSLiJ-f8SHW0L-863Q517A@xxxxxxxxxxxxxx>>>
>      >      >> Content-Type: text/plain; charset="utf-8"
>      >      >>
>      >      >> Hi,
>      >      >> Yes the method is fragile. At the time of development I
>     think it was
>      >      >> proposed to pass a struct containing a string and the void
>      >     pointer where
>      >      >> the string could be used as a identifier. But that was
>     voted down.
>      >      >> Regards
>      >      >> Anders
>      >      >
>      >      > I wasn't around for that discussion so I don't know the
>     reasons, but
>      >      > how does this sound as a refined approach?:
>      >      >
>      >      > * Define a `dissector_data_t` that has a `guint32`
>     identifier field,
>      >      > and a `void *` data field.
>      >      >
>      >      > * Replace the `void *data` parameter to dissectors with a
>     pointer
>      >     to a
>      >      > `dissector_data_t`.
>      >      >
>      >      > * Either:
>      >      >
>      >      >     * Easy way: maintain a static list of identifiers that
>     map to
>      >      > expected data types, or
>      >      >
>      >      >     * Have dissector X request an identifier in its
>     registration
>      >      > function for the type of data it expects, and have dissector Y
>      >     (which
>      >      > will call X) request, in its handoff function, the
>     identifier of the
>      >      > type of data it needs to pass to X.
>      >      >
>      >      > * Dissectors check for the right identifier in their
>      >      > `dissector_data_t` parameter and don't try to use it if
>     it's wrong.
>      >      >
>      >      > Thoughts?
>      >      >
>      >
>      >     I think what you suggest would be the most straightforward fix.
>      >
>      >     To avoid breaking backward compatibility and changing
>     thousands of
>      >     dissectors at the same time, both of which are highly
>     problematic, it
>      >     can be done by adding a new dissector type (like it was done with
>      >     "dissector_cb_t", only using a different signature).[1]
>      >
>      >     Also a giant static list of dissector_data_t type identifiers
>     would be
>      >     pretty clunky. I think we could recycle the protocol registration
>      >     number
>      >     for that.
>      >
>      >
>      > Perhaps I don't quite understand, but what would be the point if the
>      > protocol registration number were used? Presumably that is the
>     number
>      > for the called protocol, based on what David outlined (the called
>      > protocol registering what data it expects.) But the calling
>     dissector
>      > would always have that number (via
>     dissector_handle_get_protocol_index()
>      > ) and pass it in, which wouldn't provide any guarantee that the data
>      > passed in was the correct type than what is being done now.
>      >
>      > The only way that I see it would make sense to pass in an
>     identifier is
>      > if a protocol registers multiple data types it might expect to be
>     passed
>      > in when called from different types (whether in one dissect_proto()
>      > function or multiple ones), in which case the protocol registration
>      > number couldn't be used, or if the identifier is instead related
>     to the
>      > calling protocol and controlled by it (which is perhaps for this
>     method
>      > of calling the wrong dependency direction, unlike with dissector
>     tables
>      > where the calling protocol does control the passed data type, e.g.
>      > packet-ip always passes a ws_ip4* to the "ip proto" table or its
>      > heuristic subdissector table.)
>      >
>      > That doesn't sound like what's being proposed, though, so I am
>     confused.
>
>     The way I proposed it the identifier is related to the calling protocol
>     and controlled by it. It refers to the first suggestion above: "Easy
>     way: maintain a static list of identifiers that map to expected data
>     types"
>
>     Instead of having the biggest enum ever of static data identifiers we
>     can use the protocol registration number. There would probably be other
>     fields too, like version and flags, TBD).
>
>     Dissectors registering on the "ip.proto" table can receive a ws_ip4
>     or a
>     ws_ip6, the first byte is the type, so I don't follow your example
>     either. It's not really true that there is always a ws_ip4 pointer.
>
>
> Ah, yes, I see, packet-ipv6 calls the same ip_try_dissect() function 
> from packet-ip, only with a ws_ip6 as the void* iph parameter. So I had 
> that quite wrong, and that's a good case for why the current situation 
> is fragile, since any function called in that table (or the heuristic 
> table) can be passed either a ws_ip4 or ws_ip6 pointer. Though at least 
> as you say they can look at the first byte to determine the version (as 
> in ws_ip_protocol() and packet-tapa.c). Though in practice most of them 
> don't do anything with the passed in data, aside from ICMP, ICMP for 
> IPv6, and the IPv6 extension header types that can reasonably guarantee 
> what they're getting.
>
> The way I read the previous suggestion you responded to ("dissector X 
> request an identifier in its registration function for the type of data 
> it expects") made it sound like the called function would register the 
> type of data it expected, in which case the "list of static data 
> identifiers" would have been indexed by the called protocol, or by some 
> reduced set if some protocols expected the same data. My assumption is 
> that the motivation was for the called protocols to be able to determine 
> if the data passed in from a calling protocol (possibly a newly written 
> dissector or plugin) was the correct type without having to know 
> specifically about the calling protocol, even in the case of the 
> dissector being a newly written plugin with a previously unknown 
> protocol registration id. I suppose that could still be possible that way.

I wasn't responding to that because I don't understand what the second 
point is all about:

   "Have dissector X request an identifier in its registration function
   for the type of data it expects, and have dissector Y (which will
   call X) request, in its handoff function, the identifier of the type
   of data it needs to pass to X).

The calling function (dissector Y) needn't be concerned with the details 
above in the call stack (dissector X). The outer network layer 
(dissector X) can have knowledge of the inner layer (dissector Y) but 
the inverse isn't true. And can dissector Y even know if X will be 
called, since that depends on the packet contents? Maybe I misunderstood 
that.


> ___________________________________________________________________________
> 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