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.
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?
Thanks in advanced. asked 18 Apr '16, 09:33  coloncm edited 19 Apr '16, 04:32 |
One Answer:
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 is incorrect, and should, instead, be: so that, while code that adds to the protocol tree is inside answered 18 Apr ‘16, 10:14  Pascal Quantin edited 19 Apr ‘16, 00:42  Guy Harris ♦♦ showing 5 of 6 show 1 more comments |
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?
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.
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).
A performance change made since 2.0.1 could produce the observed behavior difference described here.
So you have verified, by checking the value of
treepassed to your dissector, that the Info column is populated with the correct value even if a non-null value is never passed totree?What’s prohibited is population of the Info column only in cases where
treeis 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.
His reference is section 2.11 paragraph 4, which, as indicated, says
which quite clearly indicates that columns should not be filled in inside code that is run only if
treeis null (or only iftreeis non-null).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.
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
treeis null or not. If you are updating it in the middle of a block of code insideif (tree) {and}, then you must, before each call that updates column information, close the existingif (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 newif (tree) {block by addingif (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.