2017-02-14 05:21:28

by Masashi Honma

[permalink] [raw]
Subject: [PATCH] iw: Fix bitrate output when no rate info found

Previously, bitrate showed uninitialized buffer when no rate info found.
This patch fixes the issue.

Signed-off-by: Masashi Honma <[email protected]>
---
station.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/station.c b/station.c
index f3e3da8..9d3eb4d 100644
--- a/station.c
+++ b/station.c
@@ -151,6 +151,10 @@ void parse_bitrate(struct nlattr *bitrate_attr, char *buf, int buflen)
if (rate > 0)
pos += snprintf(pos, buflen - (pos - buf),
"%d.%d MBit/s", rate / 10, rate % 10);
+ else {
+ snprintf(buf, buflen, "No rate info found!");
+ return;
+ }

if (rinfo[NL80211_RATE_INFO_MCS])
pos += snprintf(pos, buflen - (pos - buf),
--
2.7.4


2017-02-14 08:55:59

by Masashi Honma

[permalink] [raw]
Subject: Re: [PATCH] iw: Fix bitrate output when no rate info found

On 2017-02-14 17:35, Johannes Berg wrote:
> On Tue, 2017-02-14 at 14:21 +0900, Masashi Honma wrote:
>> Previously, bitrate showed uninitialized buffer when no rate info
>> found.
>
> When would this happen?

I could see in mesh STA connection with 11n and legacy mixed.
STA A has disable_ht=1.
STA B has disable_ht=0.

> I'm not really sure this is right - perhaps we don't have
> RATE_INFO_BITRATE(32), but still have the MCS data?

I recognized there was a issue on such a case. I will send a patch to
wpa_supplicant. Anyway, showing string message is better than showing
raw binary data.

> How about we just add "(unknown)" or so and not return here?

Yes. First time, I supposed to use "unknown". But in the function
parse_bitrate(), nla_parse_nested() returns message "failed to parse
nested rate attributes!". This explains why the bitrate is unknown. So I
used explaining message. We could see the message like this.

tx bitrate: No rate info found!
rx bitrate: 48.0 MBit/s

Masashi Honma.

2017-02-14 09:14:23

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] iw: Fix bitrate output when no rate info found

On Tue, 2017-02-14 at 17:55 +0900, Masashi Honma wrote:
> On 2017-02-14 17:35, Johannes Berg wrote:
> > On Tue, 2017-02-14 at 14:21 +0900, Masashi Honma wrote:
> > > Previously, bitrate showed uninitialized buffer when no rate info
> > > found.
> >
> > When would this happen?
>
> I could see in mesh STA connection with 11n and legacy mixed.
> STA A has disable_ht=1.
> STA B has disable_ht=0.

Interesting, ok.

>  > I'm not really sure this is right - perhaps we don't have
>  > RATE_INFO_BITRATE(32), but still have the MCS data?
>
> I recognized there was a issue on such a case. I will send a patch
> to wpa_supplicant. Anyway, showing string message is better than
> showing raw binary data.
>
> > How about we just add "(unknown)" or so and not return here?
>
> Yes. First time, I supposed to use "unknown".

Ok.

> But in the function 
> parse_bitrate(), nla_parse_nested() returns message "failed to parse 
> nested rate attributes!". This explains why the bitrate is unknown.

Yes, this is bad! But if you saw that, why did you ever get to the
below code that checked for the BITRATE(32) attributes?

> So I used explaining message. We could see the message like this.
>
> tx bitrate: No rate info found!
> rx bitrate: 48.0 MBit/s

Yeah I was just thinking we could also see

tx bitrate: (unknown)

and then if there was MCS anyway you'd see

tx bitrate: (unknown) MCS 7

or something like that?

johannes

2017-02-14 08:35:27

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] iw: Fix bitrate output when no rate info found

On Tue, 2017-02-14 at 14:21 +0900, Masashi Honma wrote:
> Previously, bitrate showed uninitialized buffer when no rate info
> found.

When would this happen?

I'm not really sure this is right - perhaps we don't have
RATE_INFO_BITRATE(32), but still have the MCS data?

How about we just add "(unknown)" or so and not return here?

johannes

2017-02-14 09:35:55

by Masashi Honma

[permalink] [raw]
Subject: Re: [PATCH] iw: Fix bitrate output when no rate info found

On 2017-02-14 18:14, Johannes Berg wrote:
> Yes, this is bad! But if you saw that, why did you ever get to the
> below code that checked for the BITRATE(32) attributes?
>
>> So I used explaining message. We could see the message like this.
>>
>> tx bitrate: No rate info found!
>> rx bitrate: 48.0 MBit/s
>
> Yeah I was just thinking we could also see
>
> tx bitrate: (unknown)
>
> and then if there was MCS anyway you'd see
>
> tx bitrate: (unknown) MCS 7
>
> or something like that?

Yes, showing information as far as possible looks good.

I will modify this patch.

Masashi Honma.

2017-02-14 09:45:18

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] iw: Fix bitrate output when no rate info found

On Tue, 2017-02-14 at 18:38 +0900, Masashi Honma wrote:
> Previously, bitrate showed uninitialized buffer when no rate info
> found.
> This patch fixes the issue.
>

Applied, thanks.

johannes

2017-02-14 09:39:09

by Masashi Honma

[permalink] [raw]
Subject: [PATCH v2] iw: Fix bitrate output when no rate info found

Previously, bitrate showed uninitialized buffer when no rate info found.
This patch fixes the issue.

Signed-off-by: Masashi Honma <[email protected]>
---
station.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/station.c b/station.c
index f3e3da8..4885dc0 100644
--- a/station.c
+++ b/station.c
@@ -151,6 +151,8 @@ void parse_bitrate(struct nlattr *bitrate_attr, char *buf, int buflen)
if (rate > 0)
pos += snprintf(pos, buflen - (pos - buf),
"%d.%d MBit/s", rate / 10, rate % 10);
+ else
+ pos += snprintf(pos, buflen - (pos - buf), "(unknown)");

if (rinfo[NL80211_RATE_INFO_MCS])
pos += snprintf(pos, buflen - (pos - buf),
--
2.7.4

2017-02-14 05:26:48

by Masashi Honma

[permalink] [raw]
Subject: Re: [PATCH] iw: Fix bitrate output when no rate info found

On 2017/02/14 14:21, Masashi Honma wrote:
> Previously, bitrate showed uninitialized buffer when no rate info found.
> This patch fixes the issue.

This is the screen shot.

Masashi Honma.


Attachments:
tx_bitrate.jpeg (60.78 kB)