Wireshark-dev: Re: [Wireshark-dev] Unit testing dissector code

From: João Valverde <joao.valverde@xxxxxxxxxxxxxxxxxx>
Date: Fri, 18 Jun 2021 22:26:41 +0100


On 15/06/21 05:02, João Valverde via Wireshark-dev wrote:


On 14/06/21 22:01, Martin Nyhus wrote:
On 05/06/2021 02:33, João Valverde wrote:
But regarding your PoC having to give extern linkage to the internal
dissector code is a big drawback IMO, even if it isn't visible in a DLL
(because we use default hidden visibility when the compiler supports it).

Maybe that could be solved by including the dissector source file in the
test source file?

After a few different attempts I've updated the merge request with your
idea, and none of the changes to the dissector code are needed anymore.

The new problem that shows up is that we now build the dissector twice,
once for the normal build and once when included in the test file, and
linking those fails because of the duplication. Unfortunately we can't
just exclude all the dissectors from the test linking as epan depends on
code in quite a few dissectors (enough that I gave up trying to make a
list of them).

Having epan depend on dissectors shouldn't happen, although in practice I think it has often been accepted as a mostly harmless expedient.

It should be fixable. I'll take a look at that.

There are only 3 dependencies of epan on dissectors that I found, none that seemed too difficult to fix:
epan -> radius_dict -> radius
epan -> wslua -> dissect_tcp_pdus -> tcp
epan -> some dcerpc dependency I didn't investigate

So I built test-packet-bt-dht.c against epan minus dissectors, that worked, the first problem that came up is that BT-DHT wants to register on the udp tables and those don't exist. But the UDP table isn't necessary to run the test, the BT-DHT dissector is being called directly.

So either we're back to having to maintain tested/untested catalogs of dissectors and load every dissector with every test... Or we have to think up some other solution for this issue. Probably that solution is to be more tolerant of registration failures at runtime for test purposes...?

The next problem will be that a lot of dissectors won't work without a valid pinfo structure.

The solution I ended up with is to split out the dissectors that are
being tested, and making a new object that doesn't include those. In the
future if someone wants to add tests for a dissector that epan depends
on this won't work, but it should be compatible with the previous PoC of
header + non-static functions, so there are options available.

If someone knows of a way to have Cmake include one object library in
another I'd like to get rid of DISSECTOR_UNTESTED, but for now I haven't
found a way to do it.

Appreciate the feedback,
Martin

___________________________________________________________________________
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