openSAFETY: Dissection for second crc as well as plausability check

This issue was migrated from bug 9261 in our old bug tracker.

Original bug information:

Reporter: Roland Knall
Status: RESOLVED FIXED
Product: Wireshark
Component: Dissection engine (libwireshark)
OS: All
Platform: All
Version: Git

Attachments:

wireshark_opensafety_v2.14.patch: openSAFETY: Dissection for second crc as well as plausability check
wireshark_opensafety_v2.14.patch: openSAFETY: Dissection for second crc as well as plausability check
wireshark_opensafety_v2.14.patch: openSAFETY: Dissection for second crc as well as plausability check
wireshark_opensafety_v2.14b.patch: openSAFETY: Dissection for second crc as well as plausability check
wireshark_opensafety_2.4c.patch: openSAFETY: Dissection for second crc as well as plausability check

Roland Knall said:

Created attachment 11756 openSAFETY: Dissection for second crc as well as plausability check

Build Information: TShark 1.11.0 (SVN Rev 52486 from master)

Copyright 1998-2013 Gerald Combs <gerald@‍wireshark.org> and contributors. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiled (64-bit) with GLib 2.36.0, with libpcap, with libz 1.2.7, with POSIX capabilities (Linux), without libnl, with SMI 0.4.8, with c-ares 1.9.1, with Lua 5.1, without Python, with GnuTLS 2.12.23, with Gcrypt 1.5.0, with MIT Kerberos, with GeoIP.

Running on Linux 3.8.0-29-generic, with locale de_DE.UTF-8, with libpcap version 1.3.0, with libz 1.2.7. Intel(R) Core(TM)2 Quad CPU Q8400 @ 2.66GHz

Built using gcc 4.7.3.

A user may optionally enable the dissection and calculation of the second crc in the frame for SNMT, SSDO and SPDO (not SLIM SSDO atm). By doing so, the CRC will be validated and if it can not be validated, the calculated crc will be added to the output.

This implementation is done by customer demand

Code has been fuzz-tested.

added libwireshark versiondev labels

ber…@‍min… said:

I'm not an openSAFETY expert, but I have a couple of questions and suggestions regarding that attachment 11756. First, https://bugs.wireshark.org/bugzilla/attachment.cgi?id=11756&‍action=diff#‍a/epan/dissectors/packet-opensafety.c_sec3 refers to ((2^10)-1) but ^ is a bitwise xor and not exponentiation so that gets you the value of 7 instead of 1023. You probably want either ((1<<10)-1) or just 1023.

Second, the first CRC has opensafety.crc.data and opensafety.crc.type fields as well as opensafety.crc.valid field. Should those other fields exist for the second crc as well?

Third, it would be most useful to attach a sample trace to this bug report so that your enhancements can be more easily seen and tested.

Thanks for your contribution!

Roland Knall said:

(In reply to comment #1)  
> Second, the first CRC has opensafety.crc.data and opensafety.crc.type fields  
> as well as opensafety.crc.valid field.  Should those other fields exist for  
> the second crc as well?  

That is ok. Data will never be searched, as it is not interesting. Type will allways be the same for both CRCs in a single package, and crc2 has a second valid field. (opensafety.crc2.valid). The important information here is actually if the second one is valid, and the bytes selected when the info is added to the tree.

> Third, it would be most useful to attach a sample trace to this bug report  
> so that your enhancements can be more easily seen and tested.  

This patch can be tested by using any of the other opensafety traces already in the system. But most importantly, it can easily be tested using the sample traces from: http://wiki.wireshark.org/SampleCaptures#openSAFETY

Roland Knall said:

Created attachment 11759
openSAFETY: Dissection for second crc as well as plausability check

> I'm not an openSAFETY expert, but I have a couple of questions and  
> suggestions regarding that attachment 11756 [details].  First,  
> https://bugs.wireshark.org/bugzilla/attachment.cgi?id=11756&action=diff#a/  
> epan/dissectors/packet-opensafety.c_sec3 refers to ((2^10)-1) but ^ is a  
> bitwise xor and not exponentiation so that gets you the value of 7 instead  
> of 1023.  You probably want either ((1<<10)-1) or just 1023.  

Fixed in updated patch.

Roland Knall said:

Created attachment 11760 openSAFETY: Dissection for second crc as well as plausability check

Upload went wrong

Roland Knall said:

Created attachment 11764 openSAFETY: Dissection for second crc as well as plausability check

Reworked the patch a little bit. Dissection is now smoother, and SLIM SSDOs are decoded as well.

Michael Mann said:

Comment on attachment 11764 openSAFETY: Dissection for second crc as well as plausability check

Overall patch looks fine, but it introduces tabs for whitespace when rest of the file only uses spaces.

Roland Knall said:

Created attachment 11781 openSAFETY: Dissection for second crc as well as plausability check

Roland Knall said:

(In reply to comment #6)  
> Comment on attachment 11764 [details]  
> openSAFETY: Dissection for second crc as well as plausability check  
>  
> Overall patch looks fine, but it introduces tabs for whitespace when rest of  
> the file only uses spaces.  

I really want this patch ;-) Updated it to better whitespace formatting.

Michael Mann said:

Comment on attachment 11781 openSAFETY: Dissection for second crc as well as plausability check

Committed to rev 52582.

closed