Open-Markets-Initiative / wireshark-lua

Source generated cross platform Wireshark dissectors
GNU General Public License v3.0
188 stars 63 forks source link

Cme/iLink3 Dissector - Fix Quote Cancel (528) and Quote Cancel Ack (563) #16

Closed Tzadiko closed 4 years ago

Tzadiko commented 4 years ago

Hi,

Quote Cancel and Quote Cancel Ack Fix

I'm submitting a fix for Quote Cancel (528) and Quote Cancel Ack (563) messages (sample file attached). Please note that in the attached file, the fourth frame of Quote Cancel Ack 563 is split between packet 4 and 5. githubsample.zip

Previous Errors As of commit e15ae1c Quote Cancel and Quote Cancel Ack implement Mass Quote's quote_entries_groups/quote_entries_group and quote_sets_groups/quote_sets_group. This leads to the following error for Quote Cancel and Quote Cancel Ack messages:

Quote Cancel image

Quote Cancel Ack image

Quote Cancel Ack and Quote Cancel's Entries and Sets are different from Mass Quote, for instance, Mass Quote's quote_entries_group(s) is 38 bytes long, while Quote Cancel's is 10 bytes. This leads to the dissector throwing an out of range exception.

Post-Fix These two messages are now parsed correctly by the dissector.

Quote Cancel image

Quote Cancel Ack image

Additional Minor Changes

Delay to Time

For some reason Delay To Time was not showing as "No Value" when its value is null.

Broken:

display.delay_to_time = function(value)
  -- Check if field has value
  if (value == 18446744073709551615) then
    return "Delay To Time: No Value ("..value..")"
  end

  return "Delay To Time: "..value -- Returning this
end

image

Fix:

-- Display: Delay To Time
display.delay_to_time = function(value)
  -- Check if field has value
  if (value > 18446744073709551614) then
    return "Delay To Time: No Value ("..value..")" -- Returning this.
  end

  return "Delay To Time: "..value
end

image

No Processed Quotes Size

The no_processed_entries field for Mass Quote Ack is 1 byte, but for Quote Cancel Ack it's 4 bytes. I added a no_processed_entries_563 field.

Quote Cancel Ack:

<field name="NoProcessedEntries" id="9772" type="uInt32" description="Number of quotes successfully cancelled (if in response to a Quote Cancel message)" offset="333" semanticType="int"/>

Mass Quote Ack:

<field name="NoProcessedEntries" id="9772" type="uInt8" description="Number of quotes that have been accepted from the corresponding inbound message" offset="335" semanticType="int"/>
Open-Markets-Initiative commented 4 years ago

Thanks, we will take a look. This Cme Sbe the dissector is generated directly from the exchange xml...we generally run an analysis on the generated protocols for this type of problem, but our analyzer is out of commission while we rebuild the compiler. It would have caught that they used the same identifier for 2 different groups.

Thanks for the sample pcap.

Tzadiko commented 4 years ago

Great, looking forward to hearing back.

Tzadiko commented 4 years ago

Just an FYI, the relevant frames in the sample pcap are frames 3 (Quote Cancel) and 4 (Quote Cancel Ack).

Please note that frame 4 in the pcapng file has four Quote Cancel Ack messages inside of it. Three of the four Quote Cancel Acks are parsed correctly now where previously none of them were. The error still appears for the fourth Quote Cancel Ack message since we have NetAdapterRSC enabled and the fourth Quote Cancel Ack message is split between frame 4 and 5 which isn't a behavior these dissectors are built to deal with. As a result, this error would be specific to how we capture packets and isn't a result of the dissector (so you can ignore it).

image

Open-Markets-Initiative commented 4 years ago

I added your changes as a temporary update...I am going to decide how best to regenerate. Thanks again and the problem description was great.

Open-Markets-Initiative commented 4 years ago

Please check out latest commit with regeneration around these non unique groups.

Nullable value will likely still be ignored but at this point I feel that is CME error. I will take a closer look later.

Please let me know if you see anything else.

Tzadiko commented 4 years ago

Please check out latest commit with regeneration around these non unique groups.

groups and sets for Quote Cancel Ack generate nicely now, but I still get the out of range error for Quote Cancel Ack because tot_no_quote_entries is still one byte for Quote Cancel Ack when it should be 4 bytes.

image

When I make a seperate 4-byte tot_no_quote_entries for Quote Cancel Ack (tot_no_quote_entries_563) it works.

image

What were you referring to when you mention the following?

Nullable value will likely still be ignored but at this point I feel that is CME error.

Open-Markets-Initiative commented 4 years ago

First, in QuoteCancelAck:

 <field name="TotNoQuoteEntries" id="304" type="uInt8NULL" description="Total number of quotes for the quote set across all messages. Will be populated only for enumerated rejects for Cancel By Instrument" offset="340" semanticType="int"/>

The type is type="uInt8NULL" which looks like 1 byte. You might report this to the CME.

For the No Value/Nullable this is the value I pull:

<type name="uInt64NULL" description="uInt64NULL" presence="optional" nullValue="18446744073709551615" primitiveType="uint64" semanticType="int"/>

This also looks like a violation of the spec (but could be a lua rounding/equivalence issue)

Tzadiko commented 4 years ago

Regarding QuoteCancel Ack, I meant to change NoProcessedEntries from uInt8 to uInt32, but accidentally changed TotNoQuoteEntries from uInt8NULL to uInt32. You're right, TotNoQuoteEntries should be uInt8NULL.

I noticed that in the newly generated code, NoProcessedEntries for QuoteCancel Ack is still uInt8, and so after reverting TotNoQuoteEntries from uInt32 to uInt8NULL, I'm still getting an error. The way I remedied this was making a separate NoProcessedEntries for QuoteCancel Ack (NoProcessedEntries563).

So as of your most recent commit, there is still an error on QutoeCancel Ack messages since this 4 byte NoProcessedEntries is not being generated.

Open-Markets-Initiative commented 4 years ago

I see here they made 2 versions of the same field with different sizes:

<field name="NoProcessedEntries" id="9772" type="uInt8" description="Number of quotes that have been accepted from the corresponding inbound message" offset="335" semanticType="int"/>

<field name="NoProcessedEntries" id="9772" type="uInt32" description="Number of quotes successfully cancelled (if in response to a Quote Cancel message)" offset="333" semanticType="int"/>

I really wish that analyzer was back up and running...but its going to be a few weeks. It looks for these cases since we generate on types.

Given the id is the same I think this is a either an oversight or a bad design. I may change it by hand but we still should let them know.

Tzadiko commented 4 years ago

I put in a pull request for the by-hand change. I'll also email them, but not sure how much sway I'll have. Let me know your email if you'd like to be CCed