Added dissection of IEEE-802.11ad (according to the 9th draft of the standard) to the current IEEE-802.11 dissector.Changes had been made to: epan/dissectors/packet-ieee80211.cThe code had passed fuzz testing.Done as a project in the Laboratory of Computer Communication & Networking (LCCN), Computer Science Department, Technion, Israel joint with Qualcomm-Israel.
Hi,It is possible, however I would have to check if the file we had worked with are Qualcomm's property before uploading it as public file on the web.(In reply to comment #1) > Hi jalil > > It is possible to attach a sample of 802.11ad ?
However, I noticed you do a lot of proto_tree_add_bool and proto_tree_add_uint with data retrieved unmodified from the tvb. In this case it is faster and more efficient to simply use proto_tree_add_item and let Wireshark handle fetching the data. For those fields that are only a few bits you can add a bitmask to the hf array entries instead of manually masking in the dissector code.
(In reply to comment #4) > Hi Jalil, in general it looks fine. > > However, I noticed you do a lot of proto_tree_add_bool and > proto_tree_add_uint with data retrieved unmodified from the tvb. In this > case it is faster and more efficient to simply use proto_tree_add_item and > let Wireshark handle fetching the data. For those fields that are only a few > bits you can add a bitmask to the hf array entries instead of manually > masking in the dissector code. > + 1Also I have some warning when i launch Wireshark with your sampleWarn Extended value string frame_type_subtype_vals forced to fall back to linear search: entry 24, value 23 < previous entry, value 362Warn Extended value string tag_num_vals forced to fall back to linear search: entry 129, value 143 < previous entry, value 221Warn Extended value string category_codes forced to fall back to linear search: entry 19, value 16 < previous entry, value 127Also checkhf is not happyUnused entry: epan/dissectors/packet-ieee80211.c, hf_ieee80211_ff_sswf_dmg_selectUnused entry: epan/dissectors/packet-ieee80211.c, hf_ieee80211_ff_sswf_sector_selectUnused entry: epan/dissectors/packet-ieee80211.c, hf_ieee80211_ff_sswf_snr_reportAlso why readd some static const true_false_string (dsss_ofdm_flags, cf_apsd_flags... )?I have some warning also with clang analyser toolspacket-ieee80211.c:13843:11: warning: Value stored to 'offset' is never readpacket-ieee80211.c:13832:17: warning: Value stored to 'offset' is never readpacket-ieee80211.c:13848:4: warning: Value stored to 'offset' is never readpacket-ieee80211.c:13862:11: warning: Value stored to 'offset' is never readpacket-ieee80211.c:13855:4: warning: Value stored to 'offset' is never readCheck also your indent (the file use 2 spaces indent) and there is some trailing whitespaces...(From Git : warning: squelched 114 whitespace errors, warning: 119 lines add whitespace errors. )About allocation_duration_base_custom function, use beacon_interval_base_custom function (rename if it is needed)Also there is some typo error : Changing control frame extension for extesion frames */ or + {"Low Power SC PHY Supported", "wlan.dmg_capa.low_power_suuported", or + add_tag_relay_capableities(tag_len, tree, tvb, &offset);About /* causes a segmentation fault when called. */, you need to "initialize" g_pinfo in dissect_dmg_beacon@@ -7569,6 +7568,8 @@ dissect_dmg_beacon(packet_info *pinfo, proto_tree *tree, tvbuff_t *t int tagged_parameter_tree_len; int current_offset = offset;+ g_pinfo = pinfo;
Thanks guys, changes will be done and re uploaded.(In reply to comment #5) > (In reply to comment #4) > > Hi Jalil, in general it looks fine. > > > > However, I noticed you do a lot of proto_tree_add_bool and > > proto_tree_add_uint with data retrieved unmodified from the tvb. In this > > case it is faster and more efficient to simply use proto_tree_add_item and > > let Wireshark handle fetching the data. For those fields that are only a few > > bits you can add a bitmask to the hf array entries instead of manually > > masking in the dissector code. > > >> + 1 > > Also I have some warning when i launch Wireshark with your sample > Warn Extended value string frame_type_subtype_vals forced to fall back to > linear search: entry 24, value 23 < previous entry, value 362 > Warn Extended value string tag_num_vals forced to fall back to linear > search: entry 129, value 143 < previous entry, value 221 > Warn Extended value string category_codes forced to fall back to linear > search: entry 19, value 16 < previous entry, value 127 > > > Also checkhf is not happy > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_dmg_select > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_sector_select > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_snr_report > > Also why readd some static const true_false_string (dsss_ofdm_flags, > cf_apsd_flags... )? > > I have some warning also with clang analyser tools > packet-ieee80211.c:13843:11: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13832:17: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13848:4: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13862:11: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13855:4: warning: Value stored to 'offset' is never read > > Check also your indent (the file use 2 spaces indent) and there is some > trailing whitespaces... > (From Git : warning: squelched 114 whitespace errors, warning: 119 lines add > whitespace errors. ) > > About allocation_duration_base_custom function, use > beacon_interval_base_custom function (rename if it is needed) > > Also there is some typo error : Changing control frame extension for > extesion frames */ or + {"Low Power SC PHY Supported", > "wlan.dmg_capa.low_power_suuported", or + > add_tag_relay_capableities(tag_len, tree, tvb, &offset); > > About /* causes a segmentation fault when called. */, you need to > "initialize" g_pinfo in dissect_dmg_beacon > @@ -7569,6 +7568,8 @@ dissect_dmg_beacon(packet_info *pinfo, proto_tree > *tree, tvbuff_t *t > int tagged_parameter_tree_len; > int current_offset = offset; > > + g_pinfo = pinfo;
Hi guys,Fixes as requested had been done, please see notes inline:(In reply to comment #5) > (In reply to comment #4) > > Hi Jalil, in general it looks fine. > > > > However, I noticed you do a lot of proto_tree_add_bool and > > proto_tree_add_uint with data retrieved unmodified from the tvb. In this > > case it is faster and more efficient to simply use proto_tree_add_item and > > let Wireshark handle fetching the data. For those fields that are only a few > > bits you can add a bitmask to the hf array entries instead of manually > > masking in the dissector code. > > >> + 1 We Changed it in all places where we hadn't modified the data.> Also I have some warning when i launch Wireshark with your sample > Warn Extended value string frame_type_subtype_vals forced to fall back to > linear search: entry 24, value 23 < previous entry, value 362 > Warn Extended value string tag_num_vals forced to fall back to linear > search: entry 129, value 143 < previous entry, value 221 > Warn Extended value string category_codes forced to fall back to linear > search: entry 19, value 16 < previous entry, value 127 Fixed.> Also checkhf is not happy > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_dmg_select > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_sector_select > Unused entry: epan/dissectors/packet-ieee80211.c, > hf_ieee80211_ff_sswf_snr_report Fixed.> Also why readd some static const true_false_string (dsss_ofdm_flags, > cf_apsd_flags... )? Please further explain of what you think the proper behaviour should be.> I have some warning also with clang analyser tools > packet-ieee80211.c:13843:11: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13832:17: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13848:4: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13862:11: warning: Value stored to 'offset' is never read > packet-ieee80211.c:13855:4: warning: Value stored to 'offset' is never read We didn't get the warnings after the changes, however please verify they are gone since we don't have any prior experience with clang tools.> Check also your indent (the file use 2 spaces indent) and there is some > trailing whitespaces... > (From Git : warning: squelched 114 whitespace errors, warning: 119 lines add > whitespace errors. ) Fixed as far as we could tell.> About allocation_duration_base_custom function, use > beacon_interval_base_custom function (rename if it is needed) The allocation_duration_base_custom behaves differently than beacon_interval_base_custom (there is a constant of 1024). We couldn't find a way to merge them to one function, due to lacking the possibilty of passing different constant to the calls (is it possible?).Since we changed to proto_tree_add_item, we needed 2 extra (very) simple custom base function that before was covered by direct manuplation of the data retrived from the tvb. Any other suggestion?> Also there is some typo error : Changing control frame extension for > extesion frames */ or + {"Low Power SC PHY Supported", > "wlan.dmg_capa.low_power_suuported", or + > add_tag_relay_capableities(tag_len, tree, tvb, &offset); Fixed.> About /* causes a segmentation fault when called. */, you need to > "initialize" g_pinfo in dissect_dmg_beacon > @@ -7569,6 +7568,8 @@ dissect_dmg_beacon(packet_info *pinfo, proto_tree > *tree, tvbuff_t *t > int tagged_parameter_tree_len; > int current_offset = offset; > > + g_pinfo = pinfo; No further use or need of g_pinfo in dissector.Thanks,Majd & Jalil
(In reply to comment #7) > > > Also why readd some static const true_false_string (dsss_ofdm_flags, > > cf_apsd_flags... )? > > Please further explain of what you think the proper behaviour should be. > Fixed by Michael> > I have some warning also with clang analyser tools > > packet-ieee80211.c:13843:11: warning: Value stored to 'offset' is never read > > packet-ieee80211.c:13832:17: warning: Value stored to 'offset' is never read > > packet-ieee80211.c:13848:4: warning: Value stored to 'offset' is never read > > packet-ieee80211.c:13862:11: warning: Value stored to 'offset' is never read > > packet-ieee80211.c:13855:4: warning: Value stored to 'offset' is never read > > We didn't get the warnings after the changes, however please verify they are > gone since we don't have any prior experience with clang tools. > I have always this warning with Clang (the offset value is never read after...)(In reply to comment #9) > In addition: > Some of the TAG_DMG_* cases appear to have overlapping offsets. Should the > boolean values really be 16 bits? > > TAG_DMG_OPERATION (and some others) seems to have a bit field of 16 bytes, > which would make the items 2 bytes, not 1. I confirmIt is not possible proto_tree_add_bitmask ? or use the some length for each hf ? (it is not easy to read)Example for Antenna Sector ID with your patch : Tag: Antenna Sector ID Tag Number: Antenna Sector ID (190) Tag length: 4 .... 0000 = Type: 0x00 .... ..00 0000 .... = Tap 1: 0x0000 0000 00.. = State 1: 0x00 0000 0000 = Tap 2: 0x00 0000 0000 = State 2: 0x00With patch (there is a bug in your bitmask i thinks...)My Patch :--- a/epan/dissectors/packet-ieee80211.c+++ b/epan/dissectors/packet-ieee80211.c@@ -13451,11 +13451,11 @@ add_tagged_field(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset break; } offset += 2;- proto_tree_add_item(tree, hf_ieee80211_tag_type, tvb, offset, 1, ENC_NA);- proto_tree_add_item(tree, hf_ieee80211_tag_tap1, tvb, offset, 2, ENC_NA);- proto_tree_add_item(tree, hf_ieee80211_tag_state1, tvb, offset+1, 1, ENC_NA);- proto_tree_add_item(tree, hf_ieee80211_tag_tap2, tvb, offset+2, 1, ENC_NA);- proto_tree_add_item(tree, hf_ieee80211_tag_state2, tvb, offset+3, 1, ENC_NA);+ proto_tree_add_item(tree, hf_ieee80211_tag_type, tvb, offset, 4, ENC_NA);+ proto_tree_add_item(tree, hf_ieee80211_tag_tap1, tvb, offset, 4, ENC_NA);+ proto_tree_add_item(tree, hf_ieee80211_tag_state1, tvb, offset, 4, ENC_NA);+ proto_tree_add_item(tree, hf_ieee80211_tag_tap2, tvb, offset, 4, ENC_NA);+ proto_tree_add_item(tree, hf_ieee80211_tag_state2, tvb, offset, 4, ENC_NA); offset += 4; break; }@@ -17041,27 +17041,27 @@ proto_register_ieee80211 (void) {&hf_ieee80211_tag_type, {"Type", "wlan.sctor_id.type",- FT_UINT8, BASE_HEX, NULL, 0x0f,+ FT_UINT32, BASE_HEX, NULL, 0x0f000000, NULL, HFILL }}, {&hf_ieee80211_tag_tap1, {"Tap 1", "wlan.sctor_id.tap1",- FT_UINT16, BASE_HEX, NULL, 0x03f0,+ FT_UINT32, BASE_HEX, NULL, 0x03f00000, NULL, HFILL }}, {&hf_ieee80211_tag_state1, {"State 1", "wlan.sctor_id.state1",- FT_UINT8, BASE_HEX, NULL, 0xfc,+ FT_UINT32, BASE_HEX, NULL, 0x00fc0000, NULL, HFILL }}, {&hf_ieee80211_tag_tap2, {"Tap 2", "wlan.sctor_id.tap2",- FT_UINT8, BASE_HEX, NULL, 0xff,+ FT_UINT32, BASE_HEX, NULL, 0x0000ff00, NULL, HFILL }}, {&hf_ieee80211_tag_state2, {"State 2", "wlan.sctor_id.state2",- FT_UINT8, BASE_HEX, NULL, 0xff,+ FT_UINT32, BASE_HEX, NULL, 0x000000ff, NULL, HFILL }}, {&hf_ieee80211_tag_allocation_id,--It should be applied to BRP_Request, SSW, SSWF, DMG Capa (octet by octet if needed), DMG Oper, Extended shcedule, sta availability, DMG_BEAM_REFINEMENTAlso always the typo about add_tag_relay_capableities+proto_tree_add_text(tree, tvb, offset, 1, "AID of NextPCP %d: %d", i, tvb_get_guint8(tvb, offset));[...]+ proto_tree_add_text(tree, tvb, offset, 1, "Remaining BI's: %d", tvb_get_guint8(tvb, offset));[...]+ proto_tree_add_text(tree, tvb, offset, 2, "Request Token: %d", tvb_get_letohs(tvb, offset));Use proto_tree_add_itemAlso why change the length of hf_ieee80211_fc_frame_type_subtype ?+ if (tag_len < 2) {+ proto_tree_add_text (tree, tvb, *offset + 2, tag_len,+ "Relay Capabilities: Error: Tag length must be 2 bytes long");+ return;Use expert_info
I have also warning about
Warn Extended value string category_codes forced to fall back to linear search: entry 19, value 16 < previous entry, value 127
(In reply to comment #14) > Hi Jalil > > Any news about fix your patch ? Hi Alexis,No not yet and honestly I don't see it coming before late July or even August.Sorry about that,Jalil
(In reply to comment #15) > (In reply to comment #14) > > Hi Jalil > > > > Any news about fix your patch ? > > Hi Alexis, > > No not yet and honestly I don't see it coming before late July or even > August. > > Sorry about that, > Jalil Hi Jalil,Any news ?
No feedback since June. Alexis and Michael, do you recall how much work it will take to make this worth committing? Is it better to leave it open or close it as LATER?
(In reply to comment #17)
> No feedback since June. Alexis and Michael, do you recall how much work it
> will take to make this worth committing? Is it better to leave it open or
> close it as LATER?
Hi Evans,
Need to replace some proto_tree_add_text by expert_info or proto_tree_add_item
and there is some unknown about "wrong bitmask/field size see comment 10"