Wireshark · Wireshark-dev: [Wireshark-dev] Bug in Wireshark Display filter engine caused by optimization of proto tree during d

[Richard Sharpe <realrichardsharpe@xxxxxxxxx>]

Hi folks,

Below are my findings on the problem I mentioned earlier under the
title of Is this a bug in the display filter engine or something I
have done wrong.

The problem is that unless the display filter explicitly mentions a
field it will usually be optimized out of the proto tree.

I would like more input on how to solve this problem.

One approach I can think if is that the Header Field abbrev field can
include fields that it depends on, eg:

    {&hf_ieee80211_ff_dmg_params_bss,
     {"BSS Type", "wlan.dmg_params.bss(radiotap.channel.freq)",
      FT_UINT8, BASE_DEC, VALS(bss_type), 0x03,
      NULL, HFILL }},

Where the field in parens specifies what other fields this on might
depend on. The filter parser would have to parse them out and include
them in the array of fields of interest.

However, I wonder if there is an easier way.

This only seems to be a problem for protocols that depend in some way
on protocols above them.

---------- Forwarded message ----------
From: Richard Sharpe <realrichardsharpe@xxxxxxxxx>
Date: Thu, Aug 20, 2015 at 8:57 PM
Subject: Re: [Wireshark-dev] Is this a bug in display filter engine or
something I have done wrong?
To: Gilbert Ramirez <gram@xxxxxxxxxxxxxxx>
Cc: Gerald Combs <Gerald.Combs@xxxxxxxxxxxx>


On Thu, Aug 20, 2015 at 8:13 PM, Richard Sharpe
<realrichardsharpe@xxxxxxxxx> wrote:
> On Wed, Aug 19, 2015 at 11:14 AM, Richard Sharpe
> <realrichardsharpe@xxxxxxxxx> wrote:
>> On Wed, Aug 19, 2015 at 9:37 AM, Gilbert Ramirez <gram@xxxxxxxxxxxxxxx> wrote:
>>> Actually, I don't know. There's been a lot of changes in proto.c related to
>>> PROTO_REGISTRAR_GET_NTH.
>>>
>>> In dfilter.c there is a call to proto_tree_prime_hfid() which seems to want
>>> to take some shortcuts when possible, as you suspect. And that could be the
>>> issue.
>>
>> OK, thanks. Let me look at it tonight to see if I can come up with a
>> way to short-circuit the short-cuts when we need to.
>
> OK, I think this, from epan/proto.c, might be the culprit:
>
> #define TRY_TO_FAKE_THIS_ITEM_OR_FREE(tree, hfindex, hfinfo, free_block) \
>         /* If this item is not referenced we don't have to do much work \
>            at all but we should still return a node so that field items \
>            below this node (think proto_item_add_subtree()) will still  \
>            have somewhere to attach to or else filtering will not work  \
>            (they would be ignored since tree would be NULL).            \
>            DON'T try to fake a node where PTREE_FINFO(tree) is NULL     \
>            since dissectors that want to do proto_item_set_len() or     \
>            other operations that dereference this would crash.          \
>            We fake FT_PROTOCOL unless some clients have requested us    \
>            not to do so.                                                \
>         */                                                              \
>         if (!tree) {                                                    \
>                 free_block;                                             \
>                 return NULL;                                            \
>         }                                                               \
>         PTREE_DATA(tree)->count++;                                      \
>         if (PTREE_DATA(tree)->count > MAX_TREE_ITEMS) {                 \
>                 free_block;                                             \
>                 if (getenv("WIRESHARK_ABORT_ON_TOO_MANY_ITEMS") != NULL) \
>                         g_error("More than %d items in the tree --
> possible infinite loop", MAX_TREE_ITEMS); \
>                 /* Let the exception handler add items to the tree */   \
>                 PTREE_DATA(tree)->count = 0;                            \
>                 THROW_MESSAGE(DissectorError,                           \
>                         wmem_strdup_printf(wmem_packet_scope(), "More
> than %d items in the tree -- possible infinite loop",
> MAX_TREE_ITEMS)); \
>         }                                                               \
>         PROTO_REGISTRAR_GET_NTH(hfindex, hfinfo);                       \
>         if (!(PTREE_DATA(tree)->visible)) {                             \
>                 if (PTREE_FINFO(tree)) {                                \
>                         if ((hfinfo->ref_type != HF_REF_TYPE_DIRECT)    \
>                             && (hfinfo->type != FT_PROTOCOL ||          \
>                                 PTREE_DATA(tree)->fake_protocols)) {    \
>                                 free_block;                             \
>                                 /* just return tree back to the caller */\
>                                 return tree;                            \
>                         }                                               \
>                 }                                                       \
>         }

OK, I can see what is going on. Here is the definition of proto_tree_add_uint:

/* Add FT_UINT{8,16,24,32} to a proto_tree */
proto_item *
proto_tree_add_uint(proto_tree *tree, int hfindex, tvbuff_t *tvb, gint start,
                    gint length, guint32 value)
{
        proto_item        *pi = NULL;
        header_field_info *hfinfo;

        TRY_TO_FAKE_THIS_ITEM(tree, hfindex, hfinfo);

        switch (hfinfo->type) {
                case FT_UINT8:
                case FT_UINT16:
                case FT_UINT24:
                case FT_UINT32:
                case FT_FRAMENUM:
                        pi = proto_tree_add_pi(tree, hfinfo, tvb,
start, &length);
                        proto_tree_set_uint(PNODE_FINFO(pi), value);
                        break;

                default:
                        DISSECTOR_ASSERT_NOT_REACHED();
        }

        return pi;
}

In the case of a dissection during processing of a filter expression,
if the field is not interesting it is not placed into the proto tree.

It seems like this is perhaps the only case where a protocol need
access to fields earlier in the tree (the IEEE802.11AD committee were
a little deficient there, I think).

I wonder if I could extend the abbrev field in the header_field_info
to include the other fields this one depends on. Eg:

 "some-name", "soname(proto.other.field)" ...

However, it might be that there is an easier way to do this ...

--
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)


-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)