This is our old Q&A Site. Please post any new questions and answers at ask.wireshark.org.
0
1

Greetings,

I'm noticing that the QT info column of Wireshark v2.0.2 have changed to NOT update after a tree is passed unto the dissector. In looking at the code, the info seen in Qt (left) is populated prior to checking if there is a tree node to populate (as also seen in the display window). The Gtk version of Wireshark 2.0.2 (right) updates the info column after populating the tree.

QT vs GTK

This is also observed in Wireshark version 2.0.1, but not in version 2.0.0. Is this a bug or an API change?

alt text

Thanks in advanced.

asked 18 Apr '16, 09:33

coloncm's gravatar image

coloncm
7681115
accept rate: 66%

edited 19 Apr '16, 04:32


Hi,

This is a bug in your code. Columns should not be filled under a tree presence condition, as indicated in the documentation doc/README.dissector:

"Note, however, that you must fill in column information, create conversations, reassemble packets, do calls to "expert" functions, build any other persistent state needed for dissection, and call subdissectors regardless of whether "tree" is NULL or not."

The fact that it works with GTK UI is by chance and not by design.

So, for example, code that reads like

if (tree) {

    ...

    proto_tree_add_XXX(...);

    ...

    col_add_fstr(pinfo->cinfo, COL_PROTOCOL, "Xyzzy (%u)", code);

    ...

    proto_tree_add_xxx(...);
}

is incorrect, and should, instead, be:

if (tree) {

    ...

    proto_tree_add_XXX(...);

    ...
}

col_add_fstr(pinfo->cinfo, COL_PROTOCOL, "Xyzzy (%u)", code);

if (tree) {
    ...

    proto_tree_add_xxx(...);
}

so that, while code that adds to the protocol tree is inside if (tree) { and }, and is executed only if tree is not null, code that sets or updates the column information is not inside if (tree) { and } and is executed regardless of whether tree is null or not.

permanent link

answered 18 Apr '16, 10:14

Pascal%20Quantin's gravatar image

Pascal Quantin
5.5k1060
accept rate: 30%

edited 19 Apr '16, 00:42

Guy%20Harris's gravatar image

Guy Harris ♦♦
17.4k335196

Sorry, but your extract says "regardless of whether "tree" is NULL or not", which means to me that it should not matter whether or not I have a tree presence condition when populating info columns. Howbeit, how do you explain that it worked on the QT interfaces of versions 2.0.0 and 2.0.1? Were those bugs?

(18 Apr '16, 10:43) coloncm

You get it wrong: it means that columns must be set whatever the tree state, and all dissectors are written this way (if not it is a bug). We did numerous fixes and performance enhancements since 2.0 which explain the behavior difference.

(18 Apr '16, 11:52) Pascal Quantin

I'm not certain you've captured what is happening here.

First of all, I said that my dissector, which has been maintained since version 1.10.3, worked on versions 2.0.0 and 2.0.1, so any fixes and performance enhancements made since 2.0.0 did not produce the observed behavior difference described here.

Second, the implication of the observed behavior is that:

1) info columns are being populated "once" on a dissector pass (whether or not the tree is passed), and

2) subsequent updates to the info column to add detailed info about the parsed data is not occurring, which is in contradiction to paragraph 1.4.4(3) of README.dissector:

"... before a dissector fetches any data whatsoever from the packet (unless it's a heuristic dissector fetching data to determine whether the packet is one that it should dissect, in which case it should check, before fetching the data, whether there's any data to fetch; if there isn't, it should return FALSE), it should set the Protocol column and the Info column."

This is the case in my dissector and has nothing to do with the presence of the tree, nor prohibits population of the info column in the presence of a tree. Paragraph 1.4.4(4) of README.dissector further states:

"If the Protocol column will ultimately be set to, for example, a value containing a protocol version number, with the version number being a field in the packet, the dissector should, before fetching the version number field or any other field from the packet, set it to a value without a version number, using 'col_set_str', and should later set it to a value with the version number after it's fetched the version number."

This implies that info columns can and should be updated, if needed. This is observed in the Gtk user interface, but not on the Qt interface of Wireshark 2.0.2. Therefore, your statement that "columns should not be filled under a tree presence condition" is not indicated in your reference, and explaining the behavior as occurring "by chance" if a bit hard to swallow (let alone calling it a bug in my part).

(18 Apr '16, 16:27) coloncm
1

First of all, I said that my dissector, which has been maintained since version 1.10.3, worked on versions 2.0.0 and 2.0.1, so any fixes and performance enhancements made since 2.0.0 did not produce the observed behavior difference described here.

A performance change made since 2.0.1 could produce the observed behavior difference described here.

info columns are being populated "once" on a dissector pass (whether or not the tree is passed)

So you have verified, by checking the value of tree passed to your dissector, that the Info column is populated with the correct value even if a non-null value is never passed to tree?

This is the case in my dissector and has nothing to do with the presence of the tree, nor prohibits population of the info column in the presence of a tree.

What's prohibited is population of the Info column only in cases where tree is set.

Paragraph 1.4.4(3) of README.dissector is not talking about the entire dissector, it's talking only about the code that's run before anything is fetched from the packet.

Therefore, your statement that "columns should not be filled under a tree presence condition" is not indicated in your reference,

His reference is section 2.11 paragraph 4, which, as indicated, says

Note, however, that you must fill in column information, ... regardless of whether "tree" is NULL or not.

which quite clearly indicates that columns should not be filled in inside code that is run only if tree is null (or only if tree is non-null).

(18 Apr '16, 16:51) Guy Harris ♦♦

I think we're splitting hairs, here. I took a second look at 2.0.1 and noticed that the same behavior was occurring, but not in 2.0.0, where I discovered the difference, so I will edit my question to specifically mention that. Reading between the lines, it appears that, since 2.0.1, performance enhancements were made that prohibits the update of info columns. If that's the case, I am fine with it. But, to call out observers of behavior differences to be causers of them is unfair.

Paragraph 1.4, where I am referencing from, is titled "Functions to handle columns in the traffic summary window", which is what my question is about.

Paragraph 2.11 refers to optimizations of the entire dissector, not specifically implementation of info columns. Paragraph 4 refers to the previous paragraph (3) which states that, "in the interest of speed, avoid building a protocol tree and adding stuff to it, or even looking at any packet data needed only if you're building the protocol tree, if possible."

Again, if the API changed for the sake performance enhancements on the Qt interface, I think it at least ought to be mentioned, without the implication that someone else is doing it wrong. I might add that this does not bode well with Wireshark's full migration to Qt.

(18 Apr '16, 20:43) coloncm

I think we're splitting hairs, here.

No, but if it appears that way, the documentation is apparently unclear.

Updating the column data is permitted, as long as the code that updates it is run regardless of whether tree is null or not. If you are updating it in the middle of a block of code inside if (tree) { and }, then you must, before each call that updates column information, close the existing if (tree) { block by adding a } before the call that updates column information and, after a sequence of one or more calls that update column information, start a new if (tree) { block by adding if (tree) { after all the calls that update column information.

And, in fact, that should be generalized to include all code that "create[s] conversations, reassemble[s] packets, do[es] calls to "expert" functions, build[s] any other persistent state needed for dissection, and callps] sub dissectors" - that code must be outside if (tree) {/} blocks, so that those blocks must be closed before making such calls, and opening new blocks after marking such calls.

It is not forbidden to update column information (or do any of those other things) at particular points in the flow of the dissection process - it's just forbidden (and has always been forbidden, regardless of whether, by pure luck, it happened to work nonetheless) to do so *inside an if (tree) {/} block, so you have to make sure that those blocks don't cover code that does any of those things, by ending those blocks before doing those things.

See the example I added to the answer.

(19 Apr '16, 00:49) Guy Harris ♦♦
showing 5 of 6 show 1 more comments
Your answer
toggle preview

Follow this question

By Email:

Once you sign in you will be able to subscribe for any updates here

By RSS:

Answers

Answers and Comments

Markdown Basics

  • *italic* or _italic_
  • **bold** or __bold__
  • link:[text](http://url.com/ "title")
  • image?![alt text](/path/img.jpg "title")
  • numbered list: 1. Foo 2. Bar
  • to add a line break simply add two spaces to where you would like the new line to be.
  • basic HTML tags are also supported

Question tags:

×637
×173
×25
×8

question asked: 18 Apr '16, 09:33

question was seen: 2,836 times

last updated: 19 Apr '16, 04:32

p​o​w​e​r​e​d by O​S​Q​A