Wireshark · Wireshark-dev: Re: [Wireshark-dev] Change in wireshark[master]: USB: do not take into account implicit structure al

[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

2015-01-16 3:40 GMT+01:00 Guy Harris <guy@xxxxxxxxxxxx>:

On Jan 15, 2015, at 1:54 PM, "Pascal Quantin (Code Review)" <code-review-do-not-reply@xxxxxxxxxxxxx> wrote:

> Pascal Quantin has uploaded a new change for review.
>
>  https://code.wireshark.org/review/6560
>
> Change subject: USB: do not take into account implicit structure alignment in USB address size
> ......................................................................
>
> USB: do not take into account implicit structure alignment in USB address size

"Compound" addresses, constructed from multiple non-contiguous components, are beginning to be a bit of a problem; Michael Mann's recent changes exposed the underlying problem with them, namely that we're using C data structures as the address data for them, and C data structures might have padding between elements and the end, and

        1) we do memcmp()-based comparisons on address data in, for example, hash tables

and

        2) setting individual structure members from the components of the address doesn't set the padding, so you get valgrind warnings that can reflect real problems (wherein two addresses that are the same don't compare equal) - I checked in my changes because a regression test showed some captures being dissected differently because one version of TShark found conversation matches and another version didn't, for the same capture.

Using wmem_new0() on them didn't seem to squelch the valgrind complaints; if we need to do more, perhaps that's a sign that we need a better way to handle "compound" addresses than stuffing them into C data structures.  I don't know what the better way would be - just packing them into what amount to packed structures, for example?  (No, GCC's keywords for declaring structures packet don't suffice, as not all compilers used to compile Wireshark are GCC-compatible.)

One approach could be to specialize more the addresses_equal() function so as to check, per address type, the meaningful fields instead of the full structure (including implicit padding). Of course it will have a cost in terms of processing, but would prevent any further "hidden" regression in the future. And as the function size could grow quite significantly, it might be better not to inline it anymore.

Pascal.