Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[João Valverde via Wireshark-dev]

On 22/06/21 03:35, John Thacker wrote:



On Mon, Jun 21, 2021 at 9:28 PM João Valverde via Wireshark-dev 
mailto:wireshark-dev@wireshark.org>> 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
 > mailto:wireshark-dev@wireshark.org>
<mailto:wireshark-dev@wireshark.org
<mailto: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 mailto:a.broma...@gmail.com>
 >     <mailto:a.broma...@gmail.com <mailto:a.broma...@gmail.com>>>
 >      >> To: Developer support list for Wireshark
 >     mailto:wireshark-dev@wireshark.org>
<mailto:wireshark-dev@wireshark.org
    <mailto:wireshark-dev@wireshark.org>>>
 >      >> Subject: Re: [Wireshark-dev] Calling a dissector: Type
for data
 >      >> parameter
 >      >> Message-ID:
 >      >>
 > 
 mailto:zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@mail.gmail.com> <mailto:zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@mail.gmail.com <mailto:zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@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

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[John Thacker]

On Mon, Jun 21, 2021 at 9:28 PM João Valverde via Wireshark-dev <
wireshark-dev@wireshark.org> 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
> > mailto: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  > <mailto:a.broma...@gmail.com>>
> >      >> To: Developer support list for Wireshark
> > mailto:wireshark-dev@wireshark.org>>
> >  >> Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
> >  >> parameter
> >  >> Message-ID:
> >  >>
> >   zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@mail.gmail.com  zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@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 wh

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[João Valverde via Wireshark-dev]

On 22/06/21 01:26, John Thacker wrote:
On Mon, Jun 21, 2021 at 2:21 PM João Valverde via Wireshark-dev 
mailto: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 mailto:a.broma...@gmail.com>>
 >> To: Developer support list for Wireshark
mailto:wireshark-dev@wireshark.org>>
 >> Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
 >> parameter
 >> Message-ID:
 >>
 mailto:zdycm33pxuwtbctew7gttecslij-f8shw0l-863q5...@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.


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.




John Thacker

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
  mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[John Thacker]

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 
> >> To: Developer support list for Wireshark 
> >> Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
> >> parameter
> >> Message-ID:
> >>  >
> >> 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 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[João Valverde via Wireshark-dev]

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 
To: Developer support list for Wireshark 
Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
parameter
Message-ID:

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.


João


[1]https://gitlab.com/wireshark/wireshark/-/blob/master/epan/packet.h#L79



David


___ 


Sent via:    Wireshark-dev mailing list 
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe


___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Hardening]

Le 16/06/2021 à 16:36, David Perry a écrit :

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 

[...]


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.




Hi,

I have that example with the SSL dissector: in almost all cases when we 
call this dissector from another one, we know exactly what is the next 
dissector to call on decoded content anyway AFAICT a heuristic is the 
only way to have things work as expected, or did I missed something ?


Best regards.
--
David FORT
website: https://www.hardening-consulting.com/

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[David Perry]

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 
To: Developer support list for Wireshark 
Subject: Re: [Wireshark-dev] Calling a dissector: Type for data
parameter
Message-ID:

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?

David


___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[João Valverde via Wireshark-dev]

It would be nice to fix this in a way that could be used from Lua, to 
make Lua dissectors first-class citizens and allow them to talk to C 
dissectors (and vice-versa).


On 30/05/21 11:36, Graham Bloice wrote:
When I made that change to MQTT I failed to notice that it called 
other dissectors with different data pointers, and although 
specifically modified for SparkplugB, felt that passing the topic as 
data was sufficiently general to be useful.


As others have noted, I guess the issue here is that to be more 
specific about when to call a sub-dissector those sub-dissectors need 
to register with the "parent" dissector using arbitrary fields and 
arbitrary values to determine when and how the sub-dissector should be 
called.


A more minimal solution could be for MQTT to declare an enum of "data" 
pointer types and a special registration routine and sub-dissectors 
register with that routine passing in one of the enum types and then 
in MQTT before calling a sub-dissector check that "data pointer type" 
table.


On Sat, 29 May 2021 at 08:33, Anders Broman > wrote:


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

Den lör 29 maj 2021 09:26Guy Harris mailto:ghar...@sonic.net>> skrev:

On May 29, 2021, at 12:12 AM, Anders Broman
mailto:a.broma...@gmail.com>> wrote:

> Shouldn't the caller be calling with the right data type or
NULL? So a bug in the MQTT disector?

How can the MQTT dissector determine what the right data type
*is* - especially given that the dissectors aren't wired in,
there's a UAT preference that lets the user configure it.

This is where the current mechanism for passing data between
dissectors goes crashing to the ground.

MQTT passes a topic string, which is just a string, to the
dissectors it calls.

JSON expects to be passed a pointer to an http_message_info_t.

JSON registers its non-heuristic dissector by name, and allows
it to be used with Decode As... for UDP ports.

It might *look* safe if you check the UDP dissector and the
dissectors that use "media_type" and "grpc_message_type", but
the "registers its non-heuristic dissector by name" mean there
are no guarantees, given that another dissector that passes a
pointer to something *other* than an http_message_info_t to
dissectors that are specified by name in a UAT.

___
Sent via:    Wireshark-dev mailing list
mailto:wireshark-dev@wireshark.org>>
Archives: https://www.wireshark.org/lists/wireshark-dev

Unsubscribe:
https://www.wireshark.org/mailman/options/wireshark-dev

             mailto:wireshark-dev-requ...@wireshark.org
?subject=unsubscribe

___
Sent via:    Wireshark-dev mailing list
mailto:wireshark-dev@wireshark.org>>
Archives: https://www.wireshark.org/lists/wireshark-dev

Unsubscribe:
https://www.wireshark.org/mailman/options/wireshark-dev

             mailto:wireshark-dev-requ...@wireshark.org
?subject=unsubscribe



--
Graham Bloice
Software Developer
Trihedral UK Limited

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
  mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe


___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Graham Bloice]

When I made that change to MQTT I failed to notice that it called other
dissectors with different data pointers, and although specifically modified
for SparkplugB, felt that passing the topic as data was sufficiently
general to be useful.

As others have noted, I guess the issue here is that to be more specific
about when to call a sub-dissector those sub-dissectors need to register
with the "parent" dissector using arbitrary fields and arbitrary values to
determine when and how the sub-dissector should be called.

A more minimal solution could be for MQTT to declare an enum of "data"
pointer types and a special registration routine and sub-dissectors
register with that routine passing in one of the enum types and then in
MQTT before calling a sub-dissector check that "data pointer type" table.

On Sat, 29 May 2021 at 08:33, Anders Broman  wrote:

> 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
>
> Den lör 29 maj 2021 09:26Guy Harris  skrev:
>
>> On May 29, 2021, at 12:12 AM, Anders Broman  wrote:
>>
>> > Shouldn't the caller be calling with the right data type or NULL? So a
>> bug in the MQTT disector?
>>
>> How can the MQTT dissector determine what the right data type *is* -
>> especially given that the dissectors aren't wired in, there's a UAT
>> preference that lets the user configure it.
>>
>> This is where the current mechanism for passing data between dissectors
>> goes crashing to the ground.
>>
>> MQTT passes a topic string, which is just a string, to the dissectors it
>> calls.
>>
>> JSON expects to be passed a pointer to an http_message_info_t.
>>
>> JSON registers its non-heuristic dissector by name, and allows it to be
>> used with Decode As... for UDP ports.
>>
>> It might *look* safe if you check the UDP dissector and the dissectors
>> that use "media_type" and "grpc_message_type", but the "registers its
>> non-heuristic dissector by name" mean there are no guarantees, given that
>> another dissector that passes a pointer to something *other* than an
>> http_message_info_t to dissectors that are specified by name in a UAT.
>>
>> ___
>> Sent via:Wireshark-dev mailing list 
>> Archives:https://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>>  mailto:wireshark-dev-requ...@wireshark.org
>> ?subject=unsubscribe
>>
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>


-- 
Graham Bloice
Software Developer
Trihedral UK Limited
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Anders Broman]

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

Den lör 29 maj 2021 09:26Guy Harris  skrev:

> On May 29, 2021, at 12:12 AM, Anders Broman  wrote:
>
> > Shouldn't the caller be calling with the right data type or NULL? So a
> bug in the MQTT disector?
>
> How can the MQTT dissector determine what the right data type *is* -
> especially given that the dissectors aren't wired in, there's a UAT
> preference that lets the user configure it.
>
> This is where the current mechanism for passing data between dissectors
> goes crashing to the ground.
>
> MQTT passes a topic string, which is just a string, to the dissectors it
> calls.
>
> JSON expects to be passed a pointer to an http_message_info_t.
>
> JSON registers its non-heuristic dissector by name, and allows it to be
> used with Decode As... for UDP ports.
>
> It might *look* safe if you check the UDP dissector and the dissectors
> that use "media_type" and "grpc_message_type", but the "registers its
> non-heuristic dissector by name" mean there are no guarantees, given that
> another dissector that passes a pointer to something *other* than an
> http_message_info_t to dissectors that are specified by name in a UAT.
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Guy Harris]

On May 29, 2021, at 12:12 AM, Anders Broman  wrote:

> Shouldn't the caller be calling with the right data type or NULL? So a bug in 
> the MQTT disector?

How can the MQTT dissector determine what the right data type *is* - especially 
given that the dissectors aren't wired in, there's a UAT preference that lets 
the user configure it.

This is where the current mechanism for passing data between dissectors goes 
crashing to the ground.

MQTT passes a topic string, which is just a string, to the dissectors it calls.

JSON expects to be passed a pointer to an http_message_info_t.

JSON registers its non-heuristic dissector by name, and allows it to be used 
with Decode As... for UDP ports.

It might *look* safe if you check the UDP dissector and the dissectors that use 
"media_type" and "grpc_message_type", but the "registers its non-heuristic 
dissector by name" mean there are no guarantees, given that another dissector 
that passes a pointer to something *other* than an http_message_info_t to 
dissectors that are specified by name in a UAT.
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Uli Heilmeier]

Makes sense for me. I will change the MQTT dissector to call 
call_dissector_with_data() only for sparkplug payload.

> Am 29.05.2021 um 09:12 schrieb Anders Broman  >:
> 
> Hi,
> Shouldn't the caller be calling with the right data type or NULL? So a bug in 
> the MQTT disector?
> Regards
> Anders
> 
> 
> Den lör 29 maj 2021 09:07Uli Heilmeier  > skrev:
> With MR 2706 [1] the MQTT dissector calls now subdissectors with 
> call_dissector_with_data() [2]. Previously this was call_dissector().
> 
> With this change the JSON dissector crashes WS with a memory access segfault 
> (while using MQTT decode topic as option).
> This is because for JSON we expect http_message_info_t for data pointer [3]. 
> MQTT hands over a pointer to a char array (topic_str) for data.
> 
> How do we handle data type for the data parameter pointer? Would it make 
> sense to compare at least the size of data with the size of 
> http_message_info_t?
> 
> I don’t know any way to check the data type of a pointer address location.
> 
> Cheers
> Uli
> 
> [1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/ 
> 
> [2]: 
> https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
>  
> 
> [3]: 
> https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
>  
> 
> ___
> Sent via:Wireshark-dev mailing list  >
> Archives:https://www.wireshark.org/lists/wireshark-dev 
> 
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev 
> 
>  mailto:wireshark-dev-requ...@wireshark.org 
> ?subject=unsubscribe
> ___
> Sent via:Wireshark-dev mailing list  >
> Archives:https://www.wireshark.org/lists/wireshark-dev 
> 
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev 
> 
> mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe 
> 

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Anders Broman]

Hi,
Shouldn't the caller be calling with the right data type or NULL? So a bug
in the MQTT disector?
Regards
Anders


Den lör 29 maj 2021 09:07Uli Heilmeier  skrev:

> With MR 2706 [1] the MQTT dissector calls now subdissectors with
> call_dissector_with_data() [2]. Previously this was call_dissector().
>
> With this change the JSON dissector crashes WS with a memory access
> segfault (while using MQTT decode topic as option).
> This is because for JSON we expect http_message_info_t for data pointer
> [3]. MQTT hands over a pointer to a char array (topic_str) for data.
>
> How do we handle data type for the data parameter pointer? Would it make
> sense to compare at least the size of data with the size of
> http_message_info_t?
>
> I don’t know any way to check the data type of a pointer address location.
>
> Cheers
> Uli
>
> [1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/
> [2]:
> https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
> [3]:
> https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

[Wireshark-dev] Calling a dissector: Type for data parameter

[Uli Heilmeier]

With MR 2706 [1] the MQTT dissector calls now subdissectors with 
call_dissector_with_data() [2]. Previously this was call_dissector().

With this change the JSON dissector crashes WS with a memory access segfault 
(while using MQTT decode topic as option).
This is because for JSON we expect http_message_info_t for data pointer [3]. 
MQTT hands over a pointer to a char array (topic_str) for data.

How do we handle data type for the data parameter pointer? Would it make sense 
to compare at least the size of data with the size of http_message_info_t?

I don’t know any way to check the data type of a pointer address location.

Cheers
Uli

[1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/
[2]: 
https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
[3]: 
https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe