Wireshark-bugs: [Wireshark-bugs] [Bug 8489] Dissector for NASDAQ's SoupBinTCP 3.0 protocol
Comment # 5
on bug 8489
from Evan Huus
(In reply to comment #3)
> (In reply to comment #1)
> > Quick review:
> > 1) CMakeLists.txt has the wrong filename added
> > 2) You could use the PINFO_FD_VISITED() macro
> > 3) Never use g_assert in a dissector, use DISSECTOR_ASSERT() macro instead
> > 4) never wrap try_conversation_dissector() or dissector_try_heuristic() in
> > an 'if (tree)' conditional
>
> Thanks for the feedback. 1-3 seem simple; new patch to follow.
>
> Re: #4, the switch cases for 'U' and 'S' indicate an encapsulated message.
> Am I right in thinking that instead of creating the new_tvb inside the 'if
> (tree)' block, I should retest for those cases after the 'if (tree)' block,
> and call the sub-dissector then?
I think that would work? In general, the *only* things that should happen
inside an if (tree) block are tree-related things (proto_tree_add_item etc).
Anything else (setting column text, calling subdissectors, adding expert info,
etc) should happen regardless.
A long time ago, calling proto_tree_add_item etc with a null tree would crash
and so the if (tree) checks were necessary. Now those functions are no-ops when
given null, so the checks are simply optimizations to speed up dissection. It's
perfectly valid to simply remove the if(tree) check completely and not think
too hard about it :)
(In reply to comment #4)
> (In reply to comment #2)
> > Jaap's points are correct. Also:
> >
> > - there's no need to assert the results of conversation_new or wmem_alloc at
> > all, those functions never return null
> >
> > - please attach a sample capture to test with (and fuzz-testing it yourself
> > is recommented, see http://wiki.wireshark.org/FuzzTesting)
>
> I'll remove the assertions. There's something about assuming memory
> allocation works that just scares me :-)
I know the feeling. Both of those functions are backed by glib's g_malloc,
which provides the success guarantee. If the underlying malloc fails it already
just prints a message and aborts. Very convenient :)
You are receiving this mail because:
- You are watching all bug changes.
