Merge branch 'win-fix-connectivity-logging'

4 files changed, 128 insertions, 121 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b6e30b3493..52f350561f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,10 @@ Line wrap the file at 100 chars.                                              Th
 
 
 ## [Unreleased]
+### Fixed
+#### Windows
+- Improve offline detection logic.
+
 ### Security
 #### macOS
 - Ship native Node modules unpacked to prevent malware checks by macOS on each run. The malware
diff --git a/talpid-core/src/offline/windows.rs b/talpid-core/src/offline/windows.rs
index 256fccfdde..1b685fe8c9 100644
--- a/talpid-core/src/offline/windows.rs
+++ b/talpid-core/src/offline/windows.rs
@@ -56,7 +56,7 @@ unsafe impl Send for BroadcastListener {}
 impl BroadcastListener {
     pub fn start(sender: Weak<UnboundedSender<TunnelCommand>>) -> Result<Self, Error> {
         let mut system_state = Arc::new(Mutex::new(SystemState {
-            network_connectivity: false,
+            network_connectivity: None,
             suspended: false,
             daemon_channel: sender,
         }));
@@ -205,7 +205,7 @@ impl BroadcastListener {
 
     pub fn is_offline(&self) -> bool {
         let state = self._system_state.lock();
-        state.is_offline_currently()
+        state.is_offline_currently().unwrap_or(false)
     }
 }
 
@@ -227,7 +227,7 @@ enum StateChange {
 }
 
 struct SystemState {
-    network_connectivity: bool,
+    network_connectivity: Option<bool>,
     suspended: bool,
     daemon_channel: Weak<UnboundedSender<TunnelCommand>>,
 }
@@ -237,7 +237,7 @@ impl SystemState {
         let old_state = self.is_offline_currently();
         match change {
             StateChange::NetworkConnectivity(connectivity) => {
-                self.network_connectivity = connectivity;
+                self.network_connectivity = Some(connectivity);
             }
 
             StateChange::Suspended(suspended) => {
@@ -248,15 +248,17 @@ impl SystemState {
         let new_state = self.is_offline_currently();
         if old_state != new_state {
             if let Some(daemon_channel) = self.daemon_channel.upgrade() {
-                if let Err(e) = daemon_channel.unbounded_send(TunnelCommand::IsOffline(new_state)) {
+                if let Err(e) = daemon_channel
+                    .unbounded_send(TunnelCommand::IsOffline(new_state.unwrap_or(false)))
+                {
                     log::error!("Failed to send new offline state to daemon: {}", e);
                 }
             }
         }
     }
 
-    fn is_offline_currently(&self) -> bool {
-        !self.network_connectivity || self.suspended
+    fn is_offline_currently(&self) -> Option<bool> {
+        Some(!self.network_connectivity? || self.suspended)
     }
 }
 
diff --git a/windows/winnet/src/winnet/offlinemonitor.cpp b/windows/winnet/src/winnet/offlinemonitor.cpp
index 3e223de0e7..67a90d92f0 100644
--- a/windows/winnet/src/winnet/offlinemonitor.cpp
+++ b/windows/winnet/src/winnet/offlinemonitor.cpp
@@ -5,7 +5,6 @@
 #include <libcommon/string.h>
 #include <sstream>
 
-
 using namespace std::placeholders; // for _1, _2 etc.
 
 namespace
@@ -59,54 +58,79 @@ bool IsConnectedAdapter(const MIB_IF_ROW2 &iface)
 	);
 }
 
-} // anonymous namespace
-
-
-OfflineMonitor::OfflineMonitor
-(
-	std::shared_ptr<common::logging::ILogSink> logSink,
-	Notifier notifier,
-	std::shared_ptr<NetworkAdapterMonitor::IDataProvider> dataProvider
-)
-	: m_logSink(logSink)
-	, m_notifier(notifier)
-	, m_connected(false)
-	, m_netAdapterMonitor(
-		m_logSink,
-		std::bind(&OfflineMonitor::callback, this, _1, _2, _3),
-		IsConnectedAdapter,
-		dataProvider
-	)
+void LogAdapter(std::shared_ptr<common::logging::ILogSink> logSink, const MIB_IF_ROW2 &iface)
 {
-}
-
+	//
+	// Don't flood the log with garbage.
+	//
+	static const auto blacklist = std::vector<std::wstring>
+	{
+		L"WFP Native MAC Layer LightWeight Filter",
+		L"QoS Packet Scheduler",
+		L"WFP 802.3 MAC Layer LightWeight Filter",
+		L"Microsoft Kernel Debug Network Adapter",
+		L"Software Loopback Interface",
+		L"Microsoft Teredo Tunneling Adapter",
+		L"Microsoft IP-HTTPS Platform Adapter",
+		L"Microsoft 6to4 Adapter",
+		L"WAN Miniport",
+		L"WiFi Filter Driver",
+		L"Microsoft Wi-Fi Direct Virtual Adapter",
+	};
 
-OfflineMonitor::OfflineMonitor
-(
-	std::shared_ptr<common::logging::ILogSink> logSink,
-	Notifier notifier
-) : OfflineMonitor(logSink, notifier, std::make_shared<NetworkAdapterMonitor::SystemDataProvider>())
-{
-}
+	for (const auto &black : blacklist)
+	{
+		if (nullptr != wcsstr(iface.Description, black.c_str()))
+		{
+			return;
+		}
+	}
 
+	std::stringstream ss;
 
-void OfflineMonitor::callback(const std::vector<MIB_IF_ROW2> &adapters, const MIB_IF_ROW2 *, NetworkAdapterMonitor::UpdateType)
-{
-	const auto previousConnectivity = m_connected;
-	m_connected = !adapters.empty();
+	ss << "Detailed interface logging" << std::endl;
 
-	if (previousConnectivity != m_connected)
 	{
-		m_notifier(m_connected);
+		const auto s = std::wstring(L"  Alias: ").append(iface.Alias);
+		ss << common::string::ToAnsi(s) << std::endl;
+	}
 
-		if (false == m_connected)
-		{
-			LogOfflineState();
-		}
+	{
+		const auto s = std::wstring(L"  Description: ").append(iface.Description);
+		ss << common::string::ToAnsi(s) << std::endl;
 	}
+
+	ss << "  PhysicalAddressLength: " << iface.PhysicalAddressLength << std::endl;
+	ss << "  Type: " << iface.Type << std::endl;
+	ss << "  MediaType: " << iface.MediaType << std::endl;
+	ss << "  PhysicalMediumType: " << iface.PhysicalMediumType << std::endl;
+	ss << "  AccessType: " << iface.AccessType << std::endl;
+
+	//
+	// Bool cast prevents idiot stream from inserting literal 0/1.
+	//
+
+	ss << "  InterfaceAndOperStatusFlags.HardwareInterface: " << (bool)iface.InterfaceAndOperStatusFlags.HardwareInterface << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.FilterInterface: " << (bool)iface.InterfaceAndOperStatusFlags.FilterInterface << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.ConnectorPresent: " << (bool)iface.InterfaceAndOperStatusFlags.ConnectorPresent << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.NotAuthenticated: " << (bool)iface.InterfaceAndOperStatusFlags.NotAuthenticated << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.NotMediaConnected: " << (bool)iface.InterfaceAndOperStatusFlags.NotMediaConnected << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.Paused: " << (bool)iface.InterfaceAndOperStatusFlags.Paused << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.LowPower: " << (bool)iface.InterfaceAndOperStatusFlags.LowPower << std::endl;
+	ss << "  InterfaceAndOperStatusFlags.EndPointInterface: " << (bool)iface.InterfaceAndOperStatusFlags.EndPointInterface << std::endl;
+
+	ss << "  OperStatus: " << iface.OperStatus << std::endl;
+	ss << "  AdminStatus: " << iface.AdminStatus << std::endl;
+	ss << "  MediaConnectState: " << iface.MediaConnectState << std::endl;
+	ss << "  TransmitLinkSpeed: " << iface.TransmitLinkSpeed << std::endl;
+
+	ss << "  ReceiveLinkSpeed: " << iface.ReceiveLinkSpeed << std::endl;
+	ss << "  InUcastPkts:" << iface.InUcastPkts;
+
+	logSink->info(ss.str().c_str());
 }
 
-void OfflineMonitor::LogOfflineState()
+void LogAdapters(std::shared_ptr<common::logging::ILogSink> logSink)
 {
 	//
 	// There is a race condition here because logging is not done using the
@@ -115,15 +139,13 @@ void OfflineMonitor::LogOfflineState()
 	// Not much of a problem really, this is temporary logging.
 	//
 
-	m_logSink->info("Machine is offline");
-
 	MIB_IF_TABLE2 *table;
 
 	const auto status = GetIfTable2(&table);
 
 	if (NO_ERROR != status)
 	{
-		m_logSink->error("Failed to acquire list of network interfaces. Aborting detailed logging");
+		logSink->error("Failed to acquire list of network interfaces. Aborting detailed logging");
 		return;
 	}
 
@@ -134,90 +156,68 @@ void OfflineMonitor::LogOfflineState()
 		FreeMibTable(table);
 	};
 
-	m_logSink->info("Begin detailed listing of network interfaces");
+	logSink->info("Begin detailed listing of network interfaces");
 
 	for (ULONG i = 0; i < table->NumEntries; ++i)
 	{
-		const auto &iface = table->Table[i];
+		LogAdapter(logSink, table->Table[i]);
+	}
 
-		//
-		// Don't flood the log with garbage.
-		//
-		const auto blacklist = std::vector<std::wstring>
-		{
-			L"WFP Native MAC Layer LightWeight Filter",
-			L"QoS Packet Scheduler",
-			L"WFP 802.3 MAC Layer LightWeight Filter",
-			L"Microsoft Kernel Debug Network Adapter",
-			L"Software Loopback Interface",
-			L"Microsoft Teredo Tunneling Adapter",
-			L"Microsoft IP-HTTPS Platform Adapter",
-			L"Microsoft 6to4 Adapter",
-			L"WAN Miniport",
-			L"WiFi Filter Driver",
-			L"Microsoft Wi-Fi Direct Virtual Adapter",
-		};
+	logSink->info("End detailed listing of network interfaces");
+}
 
-		bool blacklisted = false;
+} // anonymous namespace
 
-		for (const auto &black : blacklist)
-		{
-			if (nullptr != wcsstr(iface.Description, black.c_str()))
-			{
-				blacklisted = true;
-				break;
-			}
-		}
+OfflineMonitor::OfflineMonitor
+(
+	std::shared_ptr<common::logging::ILogSink> logSink,
+	Notifier notifier,
+	std::shared_ptr<NetworkAdapterMonitor::IDataProvider> dataProvider
+)
+	: m_logSink(logSink)
+	, m_notifier(notifier)
+	, m_netAdapterMonitor(
+		m_logSink,
+		std::bind(&OfflineMonitor::callback, this, _1, _2, _3),
+		IsConnectedAdapter,
+		dataProvider
+	)
+{
+}
 
-		if (blacklisted)
-		{
-			continue;
-		}
+OfflineMonitor::OfflineMonitor
+(
+	std::shared_ptr<common::logging::ILogSink> logSink,
+	Notifier notifier
+) : OfflineMonitor(logSink, notifier, std::make_shared<NetworkAdapterMonitor::SystemDataProvider>())
+{
+}
 
-		std::stringstream ss;
+void OfflineMonitor::callback(const std::vector<MIB_IF_ROW2> &adapters, const MIB_IF_ROW2 *, NetworkAdapterMonitor::UpdateType)
+{
+	const auto previousConnectivity = m_connected;
+	m_connected = !adapters.empty();
 
-		ss << "Detailed interface logging" << std::endl;
-		ss << "Interface ordinal " << i << std::endl;
+	if (previousConnectivity != m_connected)
+	{
+		std::stringstream ss;
 
+		if (previousConnectivity.has_value())
 		{
-			const auto s = std::wstring(L"  Alias: ").append(iface.Alias);
-			ss << common::string::ToAnsi(s) << std::endl;
+			ss << "Connectivity changed. Machine is: " << (*m_connected ? "ONLINE" : "OFFLINE");
+			m_logSink->info(ss.str().c_str());
 		}
-
+		else
 		{
-			const auto s = std::wstring(L"  Description: ").append(iface.Description);
-			ss << common::string::ToAnsi(s) << std::endl;
+			ss << "Initial connectivity established. Machine is: " << (*m_connected ? "ONLINE" : "OFFLINE");
+			m_logSink->info(ss.str().c_str());
 		}
 
-		ss << "  PhysicalAddressLength: " << iface.PhysicalAddressLength << std::endl;
-		ss << "  Type: " << iface.Type << std::endl;
-		ss << "  MediaType: " << iface.MediaType << std::endl;
-		ss << "  PhysicalMediumType: " << iface.PhysicalMediumType << std::endl;
-		ss << "  AccessType: " << iface.AccessType << std::endl;
-
-		//
-		// Bool cast prevents idiot stream from inserting literal 0/1.
-		//
-
-		ss << "  InterfaceAndOperStatusFlags.HardwareInterface: " << (bool)iface.InterfaceAndOperStatusFlags.HardwareInterface << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.FilterInterface: " << (bool)iface.InterfaceAndOperStatusFlags.FilterInterface << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.ConnectorPresent: " << (bool)iface.InterfaceAndOperStatusFlags.ConnectorPresent << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.NotAuthenticated: " << (bool)iface.InterfaceAndOperStatusFlags.NotAuthenticated << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.NotMediaConnected: " << (bool)iface.InterfaceAndOperStatusFlags.NotMediaConnected << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.Paused: " << (bool)iface.InterfaceAndOperStatusFlags.Paused << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.LowPower: " << (bool)iface.InterfaceAndOperStatusFlags.LowPower << std::endl;
-		ss << "  InterfaceAndOperStatusFlags.EndPointInterface: " << (bool)iface.InterfaceAndOperStatusFlags.EndPointInterface << std::endl;
-
-		ss << "  OperStatus: " << iface.OperStatus << std::endl;
-		ss << "  AdminStatus: " << iface.AdminStatus << std::endl;
-		ss << "  MediaConnectState: " << iface.MediaConnectState << std::endl;
-		ss << "  TransmitLinkSpeed: " << iface.TransmitLinkSpeed << std::endl;
-
-		ss << "  ReceiveLinkSpeed: " << iface.ReceiveLinkSpeed << std::endl;
-		ss << "  InUcastPkts:" << iface.InUcastPkts;
+		if (false == *m_connected)
+		{
+			LogAdapters(m_logSink);
+		}
 
-		m_logSink->info(ss.str().c_str());
+		m_notifier(*m_connected);
 	}
-
-	m_logSink->info("End detailed listing of network interfaces");
 }
diff --git a/windows/winnet/src/winnet/offlinemonitor.h b/windows/winnet/src/winnet/offlinemonitor.h
index 230fef48d7..6219874687 100644
--- a/windows/winnet/src/winnet/offlinemonitor.h
+++ b/windows/winnet/src/winnet/offlinemonitor.h
@@ -2,6 +2,7 @@
 
 #include <libcommon/logging/ilogsink.h>
 #include <mutex>
+#include <optional>
 #include "networkadaptermonitor.h"
 
 class OfflineMonitor
@@ -19,6 +20,7 @@ public:
 		Notifier notifier,
 		std::shared_ptr<NetworkAdapterMonitor::IDataProvider> dataProvider
 	);
+
 	OfflineMonitor(std::shared_ptr<common::logging::ILogSink> logSink, Notifier notifier);
 
 private:
@@ -26,10 +28,9 @@ private:
 	std::shared_ptr<common::logging::ILogSink> m_logSink;
 	Notifier m_notifier;
 
-	bool m_connected;
-	NetworkAdapterMonitor m_netAdapterMonitor;
+	std::optional<bool> m_connected;
 
-	void LogOfflineState();
+	NetworkAdapterMonitor m_netAdapterMonitor;
 
 	void callback(const std::vector<MIB_IF_ROW2> &adapters, const MIB_IF_ROW2 *adapter, NetworkAdapterMonitor::UpdateType type);
 };