Skip to content

Commit

Permalink
Check WPA supplicant state before saving config (#22895)
Browse files Browse the repository at this point in the history 
* Check WPA supplicant state before saving config

* Add save config log message

* Fix segfault when WiFi is not enabled but ble-wifi pairing is used

* Fix typo in variable name associattion -> association

* C++ initialization for GDBusWpaSupplicant struct

* Add missing mWpaSupplicantMutex locks

---------

Co-authored-by: Andrei Litvin <[email protected]>
  • Loading branch information
@arkq @andy31415 @pull
2 people authored and pull[bot] committed Feb 22, 2024
1 parent 8bf65e6 commit 2410340
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 49 deletions.
48 changes: 33 additions & 15 deletions src/platform/Linux/ConnectivityManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ void ConnectivityManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)
}

#if CHIP_DEVICE_CONFIG_ENABLE_WPA
bool ConnectivityManagerImpl::mAssociattionStarted = false;

bool ConnectivityManagerImpl::mAssociationStarted = false;
BitFlags<Internal::GenericConnectivityManagerImpl_WiFi<ConnectivityManagerImpl>::ConnectivityFlags>
ConnectivityManagerImpl::mConnectivityFlag;
struct GDBusWpaSupplicant ConnectivityManagerImpl::mWpaSupplicant;
Expand All @@ -157,6 +158,7 @@ ConnectivityManager::WiFiStationMode ConnectivityManagerImpl::_GetWiFiStationMod
{
if (mWiFiStationMode != kWiFiStationMode_ApplicationControlled)
{
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);
mWiFiStationMode = (mWpaSupplicant.iface != nullptr) ? kWiFiStationMode_Enabled : kWiFiStationMode_Disabled;
}

Expand Down Expand Up @@ -397,7 +399,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
{
if (g_strcmp0(value_str, "\'associating\'") == 0)
{
mAssociattionStarted = true;
mAssociationStarted = true;
}
else if (g_strcmp0(value_str, "\'disconnected\'") == 0)
{
Expand All @@ -409,7 +411,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
delegate->OnConnectionStatusChanged(static_cast<uint8_t>(ConnectionStatusEnum::kConnected));
}

if (mAssociattionStarted)
if (mAssociationStarted)
{
uint8_t associationFailureCause = static_cast<uint8_t>(AssociationFailureCauseEnum::kUnknown);
uint16_t status = WLAN_STATUS_UNSPECIFIED_FAILURE;
Expand Down Expand Up @@ -447,7 +449,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte

DeviceLayer::SystemLayer().ScheduleLambda([]() { ConnectivityMgrImpl().UpdateNetworkStatus(); });

mAssociattionStarted = false;
mAssociationStarted = false;
}
else if (g_strcmp0(value_str, "\'associated\'") == 0)
{
Expand All @@ -460,7 +462,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
}
else if (g_strcmp0(value_str, "\'completed\'") == 0)
{
if (mAssociattionStarted)
if (mAssociationStarted)
{
DeviceLayer::SystemLayer().ScheduleLambda([]() {
if (mpConnectCallback != nullptr)
Expand All @@ -471,7 +473,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
ConnectivityMgrImpl().PostNetworkConnect();
});
}
mAssociattionStarted = false;
mAssociationStarted = false;
}
}

Expand Down Expand Up @@ -723,14 +725,10 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe

void ConnectivityManagerImpl::StartWiFiManagement()
{
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

mConnectivityFlag.ClearAll();
mWpaSupplicant.state = GDBusWpaSupplicant::INIT;
mWpaSupplicant.scanState = GDBusWpaSupplicant::WIFI_SCANNING_IDLE;
mWpaSupplicant.proxy = nullptr;
mWpaSupplicant.iface = nullptr;
mWpaSupplicant.bss = nullptr;
mWpaSupplicant.interfacePath = nullptr;
mWpaSupplicant.networkPath = nullptr;
mWpaSupplicant = GDBusWpaSupplicant{};

ChipLogProgress(DeviceLayer, "wpa_supplicant: Start WiFi management");

Expand All @@ -752,6 +750,8 @@ void ConnectivityManagerImpl::DriveAPState()
CHIP_ERROR err = CHIP_NO_ERROR;
WiFiAPState targetState;

std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

// If the AP interface is not under application control...
if (mWiFiAPMode != kWiFiAPMode_ApplicationControlled)
{
Expand Down Expand Up @@ -867,6 +867,8 @@ CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP()
uint16_t discriminator = 0;
char ssid[32];

std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

channel = ConnectivityUtils::MapChannelToFrequency(kWiFi_BAND_2_4_GHZ, CHIP_DEVICE_CONFIG_WIFI_AP_CHANNEL);

if (GetCommissionableDataProvider()->GetSetupDiscriminator(discriminator) != CHIP_NO_ERROR)
Expand Down Expand Up @@ -961,8 +963,11 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent
char ssidStr[kMaxWiFiSSIDLength + 1u] = { 0 };
char keyStr[kMaxWiFiKeyLength + 1u] = { 0 };

std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

VerifyOrReturnError(ssid.size() <= kMaxWiFiSSIDLength, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(credentials.size() <= kMaxWiFiKeyLength, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(mWpaSupplicant.iface != nullptr, CHIP_ERROR_INCORRECT_STATE);

// There is another ongoing connect request, reject the new one.
VerifyOrReturnError(mpConnectCallback == nullptr, CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -1046,6 +1051,9 @@ void ConnectivityManagerImpl::_ConnectWiFiNetworkAsyncCallback(GObject * source_
ConnectivityManagerImpl * this_ = reinterpret_cast<ConnectivityManagerImpl *>(user_data);
std::unique_ptr<GVariant, GVariantDeleter> attachRes;
std::unique_ptr<GError, GErrorDeleter> err;

std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

{
gboolean result = wpa_fi_w1_wpa_supplicant1_interface_call_select_network_finish(mWpaSupplicant.iface, res,
&MakeUniquePointerReceiver(err).Get());
Expand Down Expand Up @@ -1132,7 +1140,15 @@ CHIP_ERROR ConnectivityManagerImpl::CommitConfig()
gboolean result;
std::unique_ptr<GError, GErrorDeleter> err;

ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to network");
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED)
{
ChipLogError(DeviceLayer, "wpa_supplicant: CommitConfig: interface proxy not connected");
return CHIP_ERROR_INCORRECT_STATE;
}

ChipLogProgress(DeviceLayer, "wpa_supplicant: save config");

result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr,
&MakeUniquePointerReceiver(err).Get());
Expand Down Expand Up @@ -1196,7 +1212,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetWiFiSecurityType(SecurityTypeEnum & secur

if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED)
{
ChipLogError(DeviceLayer, "wpa_supplicant: _GetWiFiSecurityType: interface proxy not connected");
ChipLogError(DeviceLayer, "wpa_supplicant: GetWiFiSecurityType: interface proxy not connected");
return CHIP_ERROR_INCORRECT_STATE;
}

Expand Down Expand Up @@ -1605,6 +1621,8 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi

void ConnectivityManagerImpl::_OnWpaInterfaceScanDone(GObject * source_object, GAsyncResult * res, gpointer user_data)
{
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

ChipLogProgress(DeviceLayer, "wpa_supplicant: network scan done");
gchar ** bsss = wpa_fi_w1_wpa_supplicant1_interface_dup_bsss(mWpaSupplicant.iface);
gchar ** oldBsss = bsss;
Expand Down
26 changes: 15 additions & 11 deletions src/platform/Linux/ConnectivityManagerImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace DeviceLayer {
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
struct GDBusWpaSupplicant
{
enum
enum WpaState
{
INIT,
WPA_CONNECTING,
Expand All @@ -74,19 +74,21 @@ struct GDBusWpaSupplicant
WPA_NO_INTERFACE_PATH,
WPA_GOT_INTERFACE_PATH,
WPA_INTERFACE_CONNECTED,
} state;
};

enum
enum WpaScanning
{
WIFI_SCANNING_IDLE,
WIFI_SCANNING,
} scanState;
};

WpaFiW1Wpa_supplicant1 * proxy;
WpaFiW1Wpa_supplicant1Interface * iface;
WpaFiW1Wpa_supplicant1BSS * bss;
gchar * interfacePath;
gchar * networkPath;
WpaState state = INIT;
WpaScanning scanState = WIFI_SCANNING_IDLE;
WpaFiW1Wpa_supplicant1 * proxy = nullptr;
WpaFiW1Wpa_supplicant1Interface * iface = nullptr;
WpaFiW1Wpa_supplicant1BSS * bss = nullptr;
gchar * interfacePath = nullptr;
gchar * networkPath = nullptr;
};
#endif

Expand Down Expand Up @@ -203,9 +205,11 @@ class ConnectivityManagerImpl final : public ConnectivityManager,

static bool _GetBssInfo(const gchar * bssPath, NetworkCommissioning::WiFiScanResponse & result);

static bool mAssociattionStarted;
static bool mAssociationStarted;
static BitFlags<ConnectivityFlags> mConnectivityFlag;
static struct GDBusWpaSupplicant mWpaSupplicant;
static GDBusWpaSupplicant mWpaSupplicant;
// Access to mWpaSupplicant has to be protected by a mutex because it is accessed from
// the CHIP event loop thread and dedicated D-Bus thread started by platform manager.
static std::mutex mWpaSupplicantMutex;

NetworkCommissioning::Internal::BaseDriver::NetworkStatusChangeCallback * mpStatusChangeCallback = nullptr;
Expand Down
30 changes: 17 additions & 13 deletions src/platform/webos/ConnectivityManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ void ConnectivityManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)
}

#if CHIP_DEVICE_CONFIG_ENABLE_WPA
bool ConnectivityManagerImpl::mAssociattionStarted = false;

bool ConnectivityManagerImpl::mAssociationStarted = false;
BitFlags<Internal::GenericConnectivityManagerImpl_WiFi<ConnectivityManagerImpl>::ConnectivityFlags>
ConnectivityManagerImpl::mConnectivityFlag;
struct GDBusWpaSupplicant ConnectivityManagerImpl::mWpaSupplicant;
Expand Down Expand Up @@ -393,7 +394,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
{
if (g_strcmp0(value_str, "\'associating\'") == 0)
{
mAssociattionStarted = true;
mAssociationStarted = true;
}
else if (g_strcmp0(value_str, "\'disconnected\'") == 0)
{
Expand All @@ -405,7 +406,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte
delegate->OnConnectionStatusChanged(static_cast<uint8_t>(ConnectionStatusEnum::kConnected));
}

if (mAssociattionStarted)
if (mAssociationStarted)
{
uint8_t associationFailureCause = static_cast<uint8_t>(AssociationFailureCauseEnum::kUnknown);
uint16_t status = WLAN_STATUS_UNSPECIFIED_FAILURE;
Expand Down Expand Up @@ -435,7 +436,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte

DeviceLayer::SystemLayer().ScheduleLambda([]() { ConnectivityMgrImpl().UpdateNetworkStatus(); });

mAssociattionStarted = false;
mAssociationStarted = false;
}
else if (g_strcmp0(value_str, "\'associated\'") == 0)
{
Expand Down Expand Up @@ -697,13 +698,7 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe
void ConnectivityManagerImpl::StartWiFiManagement()
{
mConnectivityFlag.ClearAll();
mWpaSupplicant.state = GDBusWpaSupplicant::INIT;
mWpaSupplicant.scanState = GDBusWpaSupplicant::WIFI_SCANNING_IDLE;
mWpaSupplicant.proxy = nullptr;
mWpaSupplicant.iface = nullptr;
mWpaSupplicant.bss = nullptr;
mWpaSupplicant.interfacePath = nullptr;
mWpaSupplicant.networkPath = nullptr;
mWpaSupplicant = GDBusWpaSupplicant{};

ChipLogProgress(DeviceLayer, "wpa_supplicant: Start WiFi management");

Expand Down Expand Up @@ -936,6 +931,7 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent

VerifyOrReturnError(ssid.size() <= kMaxWiFiSSIDLength, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(credentials.size() <= kMaxWiFiKeyLength, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(mWpaSupplicant.iface != nullptr, CHIP_ERROR_INCORRECT_STATE);

// There is another ongoing connect request, reject the new one.
VerifyOrReturnError(mpConnectCallback == nullptr, CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -1094,7 +1090,15 @@ CHIP_ERROR ConnectivityManagerImpl::CommitConfig()
gboolean result;
std::unique_ptr<GError, GErrorDeleter> err;

ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to network");
std::lock_guard<std::mutex> lock(mWpaSupplicantMutex);

if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED)
{
ChipLogError(DeviceLayer, "wpa_supplicant: CommitConfig: interface proxy not connected");
return CHIP_ERROR_INCORRECT_STATE;
}

ChipLogProgress(DeviceLayer, "wpa_supplicant: save config");

result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr,
&MakeUniquePointerReceiver(err).Get());
Expand Down Expand Up @@ -1158,7 +1162,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetWiFiSecurityType(SecurityTypeEnum & secur

if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED)
{
ChipLogError(DeviceLayer, "wpa_supplicant: _GetWiFiSecurityType: interface proxy not connected");
ChipLogError(DeviceLayer, "wpa_supplicant: GetWiFiSecurityType: interface proxy not connected");
return CHIP_ERROR_INCORRECT_STATE;
}

Expand Down
22 changes: 12 additions & 10 deletions src/platform/webos/ConnectivityManagerImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace DeviceLayer {
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
struct GDBusWpaSupplicant
{
enum
enum WpaState
{
INIT,
WPA_CONNECTING,
Expand All @@ -74,19 +74,21 @@ struct GDBusWpaSupplicant
WPA_NO_INTERFACE_PATH,
WPA_GOT_INTERFACE_PATH,
WPA_INTERFACE_CONNECTED,
} state;
};

enum
enum WpaScanning
{
WIFI_SCANNING_IDLE,
WIFI_SCANNING,
} scanState;
};

WpaFiW1Wpa_supplicant1 * proxy;
WpaFiW1Wpa_supplicant1Interface * iface;
WpaFiW1Wpa_supplicant1BSS * bss;
gchar * interfacePath;
gchar * networkPath;
WpaState state = INIT;
WpaScanning scanState = WIFI_SCANNING_IDLE;
WpaFiW1Wpa_supplicant1 * proxy = nullptr;
WpaFiW1Wpa_supplicant1Interface * iface = nullptr;
WpaFiW1Wpa_supplicant1BSS * bss = nullptr;
gchar * interfacePath = nullptr;
gchar * networkPath = nullptr;
};
#endif

Expand Down Expand Up @@ -203,7 +205,7 @@ class ConnectivityManagerImpl final : public ConnectivityManager,

static bool _GetBssInfo(const gchar * bssPath, NetworkCommissioning::WiFiScanResponse & result);

static bool mAssociattionStarted;
static bool mAssociationStarted;
static BitFlags<ConnectivityFlags> mConnectivityFlag;
static struct GDBusWpaSupplicant mWpaSupplicant;
static std::mutex mWpaSupplicantMutex;
Expand Down

0 comments on commit 2410340

Please sign in to comment.