Wireshark-bugs: [Wireshark-bugs] [Bug 7902] Improved Dissection of Modbus/TCP messages and added

Date: Mon, 22 Oct 2012 18:07:32 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7902

Michael Mann <mmann78@xxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mmann78@xxxxxxxxxxxx

--- Comment #14 from Michael Mann <mmann78@xxxxxxxxxxxx> 2012-10-22 18:07:31 PDT ---
Most of the dissection code you've added is not in the official Modbus/TCP
specification
(http://www.modbus.org/docs/Modbus_Application_Protocol_V1_1b.pdf).  I know
this particular dissection is used out in the real world, but do you have any
specifications that document it?

I had modified the Modbus/TCP dissector to be broken up into 2 separate
dissectors - Modbus/TCP + "Modbus" (PDU).  The intention of this was that
"Modbus" could be dissected with any "encapsulation" layer, be it Modbus/TCP,
Modbus RTU or another dissector (like CIP)).

The Modbus/TCP dissector dissects the MBAP Header and then sends the dissection
to the "Modbus" dissector.  If a Modbus message were coming over RTU/serial, I
would expect a third dissector.  The "Modbus RTU" dissector would dissect the
Unit ID and then pass the data on to the "Modbus" dissector.  I don't think
there should be preferences for a "serial" protocol mixed with the TCP protocol
that is Modbus/TCP.  You also then have the freedom to better guess the "packet
type" and pass it into the proto data like Modbus/TCP does.  Because the
dissector would be very small, I don't think it warrants a new file, but I
think it should at least be separated out from Modbus/TCP.

I like the TCP port preference addition as I know port 502 is reserved by IANA,
but any port could be (and is) used.  Again, however I don't see explicit
mention of the use of UDP in the Modbus specification.  It was in previous
versions of Wireshark, but I intentionally removed for lack of specification
(see bug 5923).

I like the "cleanup" of the field names, but your patch overall doesn't confirm
to the whitespace formatting (modelines) of the rest of the dissector.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.