2015-07-17 2:53 GMT+03:00 Stefan Sperling <s...@stsp.name>:
> On Fri, Jul 17, 2015 at 02:26:29AM +0300, Vadim Zhukov wrote:
>> 2015-07-17 1:49 GMT+03:00 Stefan Sperling <s...@stsp.name>:
>> > On Fri, Jul 17, 2015 at 01:23:54AM +0300, Vadim Zhukov wrote:
>> >> > + printf(", channels %d", first_chan);
>> >> > + for (i = first_chan + 1; i < nchan; i++)
>> >> > + printf(" %i", i);
>> >>
>> >> It looks like you simply enumerate from first_chan till nchan; why not
>> >
>> > Indeed, this is what the channel data means.
>> >
>> >> to just print "channels %d-%d" then? Also, with this for loop, in case
>> >> of invalid data (nchan < first_chan) you won't have any data printed
>> >> at all.
>> >
>> > Good catch.
>> >
>> > My previous diff also didn't properly loop over the channel/tx power list.
>> >
>> > New diff. Tested in hackroom, where e.g. on 5GHz I see:
>> >
>> > country 'CA ', channels 52-55 max TX power 24dB, channels 132-134 max TX
>> > power 24dB
>>
>> Looks nice for me, but two more nits are inline below.
>>
>> > Index: print-802_11.c
>> > ===================================================================
>> > RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
>> > retrieving revision 1.19
>> > diff -u -p -r1.19 print-802_11.c
>> > --- print-802_11.c 16 Jul 2015 20:57:13 -0000 1.19
>> > +++ print-802_11.c 16 Jul 2015 22:45:51 -0000
>> > @@ -78,6 +78,7 @@ int ieee80211_hdr(struct ieee80211_fram
>> > int ieee80211_data(struct ieee80211_frame *, u_int);
>> > void ieee80211_print_element(u_int8_t *, u_int);
>> > void ieee80211_print_essid(u_int8_t *, u_int);
>> > +void ieee80211_print_country(u_int8_t *, u_int);
>> > void ieee80211_print_htcaps(u_int8_t *, u_int);
>> > int ieee80211_elements(struct ieee80211_frame *, u_int);
>> > int ieee80211_frame(struct ieee80211_frame *, u_int);
>> > @@ -233,6 +234,43 @@ ieee80211_print_essid(u_int8_t *essid, u
>> >
>> > /* Caller checks len */
>> > void
>> > +ieee80211_print_country(u_int8_t *data, u_int len)
>> > +{
>> > + u_int8_t first_chan, nchan, maxpower;
>> > +
>> > + if (len < 6)
>>
>> Shouldn't this be "len < 3" here?
>
> No. The element must contain one first_chan/nchan/tx power triplet.
>
> The 801.22-2012 standard says: "The minimum length of the element is 8
> octets."
> This includes the elemend id (1 byte), length (1 byte), and data (6 bytes).
>
>> > + return;
>> > +
>> > + /* country string */
>> > + printf(" '%c%c%c'", data[0], data[1], data[2]);
>> > +
>> > + len -= 3;
>> > + data += 3;
>> > +
>> > + /* channels and corresponding TX power limits */
>> > + while (len > 0) {
>> > + if (len < 6)
>
> The above, however, needs to be len < 3 since we're just checking
> the length of the triplet here. And the += 6 increment at the end
> of the loop was wrong, too.
>
>> > + break;
>> > +
>> > + /* ignore operating extension Id (values >= 201) for now */
>> > + if (data[3] >= 201 || data[4] >= 201)
>> > + continue;
>> > +
>
> Sigh... And these indices were wrong, too:
>
>> > + first_chan = data[3];
>> > + nchan = data[4];
>> > + maxpower = data[5];
>
>> Hm, what if we have received nchan==0, maybe some warning should be
>> printed?
>
> I don't think we should bother with invalid data. If it's not valid,
> we should silently skip it. Else tcpdump would have to print all sorts
> of warnings.
Well, it depends. I've just picked print-tcp.c for example:
if (length < sizeof(*tp)) {
(void)printf("truncated-tcp %d", length);
return;
}
Or:
datalen = len - 2;
if ((datalen % TCPOLEN_SACK) != 0 ||
!(flags & TH_ACK)) {
(void)printf("malformed sack ");
(void)printf("[len %d] ", datalen);
break;
}
So at least in some cases warnings (some sort of) are welcome in tcpdump.
I don't insist on explicit warning ("invalid nchan" or something like
that), no. But, IMHO, we should display data we gathered without
mangling; otherwise, we'll fool the user that nchan is 1 when it's 0
actually. It's like gdb printing "1" for int x when x is actually 0.
That's why I insist on "nchan != 1" instead of "nchan > 1", or some
equivalent tweak like always printing the raw values - this is how
some flags fields are already printed in tcpdump code:
03:58:28.236794 802.1d RSTP config flags=3c<LEARNING,FORWARDING> ...
>> > + len -= 6;
>> > + data += 6;
>> > + }
>> > +}
>
> New diff which produces the following output in my testing:
>
> country 'CA ', channels 36-39 17dB, channels 52-55 24dB, channels 100-104
> 24dB, channels 132-134 24dB, channels 149-153 30dB
>
> Index: print-802_11.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 print-802_11.c
> --- print-802_11.c 16 Jul 2015 20:57:13 -0000 1.19
> +++ print-802_11.c 16 Jul 2015 23:49:40 -0000
> @@ -78,6 +78,7 @@ int ieee80211_hdr(struct ieee80211_fram
> int ieee80211_data(struct ieee80211_frame *, u_int);
> void ieee80211_print_element(u_int8_t *, u_int);
> void ieee80211_print_essid(u_int8_t *, u_int);
> +void ieee80211_print_country(u_int8_t *, u_int);
> void ieee80211_print_htcaps(u_int8_t *, u_int);
> int ieee80211_elements(struct ieee80211_frame *, u_int);
> int ieee80211_frame(struct ieee80211_frame *, u_int);
> @@ -233,6 +234,43 @@ ieee80211_print_essid(u_int8_t *essid, u
>
> /* Caller checks len */
> void
> +ieee80211_print_country(u_int8_t *data, u_int len)
> +{
> + u_int8_t first_chan, nchan, maxpower;
> +
> + if (len < 6)
> + return;
> +
> + /* country string */
> + printf(" '%c%c%c'", data[0], data[1], data[2]);
> +
> + len -= 3;
> + data += 3;
> +
> + /* channels and corresponding TX power limits */
> + while (len > 0) {
> + if (len < 3)
> + break;
> +
> + /* ignore operating extension Id (values >= 201) for now */
> + if (data[0] >= 201 || data[1] >= 201)
> + continue;
> +
> + first_chan = data[0];
> + nchan = data[1];
> + maxpower = data[2];
> +
> + printf(", channel%s %d", nchan > 1 ? "s" : "", first_chan);
> + if (nchan > 1)
> + printf("-%d", first_chan + nchan - 1);
> + printf(" %ddB", maxpower);
> +
> + len -= 3;
> + data += 3;
> + }
> +}
> +/* Caller checks len */
> +void
> ieee80211_print_htcaps(u_int8_t *data, u_int len)
> {
> u_int16_t htcaps;
> @@ -392,8 +430,7 @@ ieee80211_elements(struct ieee80211_fram
> break;
> case IEEE80211_ELEMID_COUNTRY:
> printf(", country");
> - for (i = len; i > 0; i--, data++)
> - printf(" %u", data[0]);
> + ieee80211_print_country(data, len);
> break;
> case IEEE80211_ELEMID_CHALLENGE:
> printf(", challenge");
> @@ -436,6 +473,9 @@ ieee80211_elements(struct ieee80211_fram
> printf(", htcaps");
> if (vflag)
> ieee80211_print_htcaps(data, len);
> + break;
> + case IEEE80211_ELEMID_POWER_CONSTRAINT:
> + printf(", power constraint %udB", data[0]);
> break;
> case IEEE80211_ELEMID_VENDOR:
> printf(", vendor");
--
WBR,
Vadim Zhukov
