Re: Calling a dissector: Type for data parameter

[John Thacker <johnthacker () gmail com>]

On Mon, Jun 21, 2021 at 2:21 PM João Valverde via Wireshark-dev <
wireshark-dev () wireshark org> 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 () gmail com>
> > > To: Developer support list for Wireshark <wireshark-dev () wireshark org>
> > > Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
> > >     parameter
> > > Message-ID:
> > >     <CAOpyz=zDycm33PXUwtBCTew7gTTEcSLiJ-f8SHW0L-863Q517A () mail gmail com
> >
> > > 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.

John Thacker

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe