New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 904021 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

shill: fix Nl80211Frame::ToString(), which has been broken since CL:289228

Project Member Reported by benchan@google.com, Nov 10

Issue description

Nl80211Frame::ToString() has been broken since CL:289228, which introduced a bug where |Nl80211Frame::reason_code_string_| and |Nl80211Frame::status_code_string_| became never initialized.

 
Cc: briannorris@chromium.org kirtika@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8374b8c4012136eed7ce1f21dfb7bb50c3865277

commit 8374b8c4012136eed7ce1f21dfb7bb50c3865277
Author: Ben Chan <benchan@chromium.org>
Date: Thu Nov 15 10:17:05 2018

shill: fix issues in Nl80211Frame::ToString()

This CL addresses the following issues with Nl80211Frame:

- Nl80211Frame::ToString() is broken since CL:289228, which introduced a
  bug where |Nl80211Frame::reason_code_string_| and
  |Nl80211Frame::status_code_string_| became never initialized.

- The code relies on Nl80211Frame::InitFromPacket() to initialize
  |Nl80211Frame::reason_code_string_| and
  |Nl80211Frame::status_code_string_|, which doesn't guarantee that they
  aren't accessed before Nl80211Frame::InitFromPacket() is invoked.

- Nl80211Frame::StringFromReason() and Nl80211Frame::StringFromReason()
  are implementation details of Nl80211Frame. It's unnecessary to expose
  them through the class definition of Nl80211Frame. Instead, these
  helpers are better defined under the IEEE_80211 namespace.

BUG= chromium:904021 
TEST=Run unit tests.

Change-Id: I952ac6d545f487c726878084a180d5a1c8cf5d69
Reviewed-on: https://chromium-review.googlesource.com/1330124
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>

[add] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/net/ieee80211.cc
[modify] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/net/nl80211_message.h
[modify] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/BUILD.gn
[add] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/net/nl80211_message_test.cc
[modify] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/net/ieee80211.h
[modify] https://crrev.com/8374b8c4012136eed7ce1f21dfb7bb50c3865277/shill/net/nl80211_message.cc

Status: Fixed (was: Started)
My fuzzer was going to catch this one :D
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8eb370f60f962801a98efce8dd3b3ec84048ebd0

commit 8eb370f60f962801a98efce8dd3b3ec84048ebd0
Author: Brian Norris <briannorris@chromium.org>
Date: Sat Dec 08 16:44:03 2018

common-mk: build with -Wunreachable-code

This warning is not included in any of the default -Wall / -Wextra
warning lists for clang at the moment (and it does nothing on gcc [1]),
but it would have caught chromium:904021 a long time ago. Let's enable
it by default.

[1] https://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html

CQ-DEPEND=CL:1351683, CL:1351840
BUG= chromium:904021 
TEST=build shill, with and without unreachable code; precq

Change-Id: Ibf715a2e9cff896c1e87a1f46efa1b747065312e
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1330972
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/8eb370f60f962801a98efce8dd3b3ec84048ebd0/common-mk/common.mk
[modify] https://crrev.com/8eb370f60f962801a98efce8dd3b3ec84048ebd0/common-mk/BUILD.gn
[modify] https://crrev.com/8eb370f60f962801a98efce8dd3b3ec84048ebd0/common-mk/common.gypi

Sign in to add a comment