From 214f856512de2835bb3fa27a2e63fd7eaef4054e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Saliba?= Date: Sat, 18 Mar 2023 12:26:45 +0100 Subject: [PATCH] Avoided a deadlock in DNS resolution. Minor refactoring and warnings resolutions. --- Common/IO/Files/PathResolver.cs | 1 - Common/Net/DNS/ResolvedIPInformation.cs | 56 +++++++++++++++------- Common/Processes/ProcessHelper.cs | 14 +++++- Common/UI/ViewModels/ConnectionBaseInfo.cs | 1 - Common/UI/ViewModels/LogEntryViewModel.cs | 2 +- Console/UI/Controls/BandwidthGraph.xaml.cs | 2 +- Console/UI/Controls/Map.xaml.cs | 8 ++-- Console/UI/Pages/AdapterInfo.xaml | 8 ++-- Console/UI/Pages/Connections.xaml | 43 +++++++++++++++-- Console/UI/Pages/Rules.xaml.cs | 2 +- Console/UI/Pages/TimerBasedPage.cs | 2 +- Console/UI/Windows/MainWindow.xaml | 4 +- Console/UI/Windows/MainWindow.xaml.cs | 22 ++++----- Console/ViewModels/ExposedInterfaceView.cs | 4 +- Console/ViewModels/GeoConnection2.cs | 33 ++++++------- 15 files changed, 134 insertions(+), 68 deletions(-) diff --git a/Common/IO/Files/PathResolver.cs b/Common/IO/Files/PathResolver.cs index bea31e4..b7f7706 100644 --- a/Common/IO/Files/PathResolver.cs +++ b/Common/IO/Files/PathResolver.cs @@ -5,7 +5,6 @@ using System.Linq; using System.Runtime.InteropServices; using System.Text; - using Wokhan.WindowsFirewallNotifier.Common.Logging; namespace Wokhan.WindowsFirewallNotifier.Common.IO.Files; diff --git a/Common/Net/DNS/ResolvedIPInformation.cs b/Common/Net/DNS/ResolvedIPInformation.cs index 154d88a..2b1a742 100644 --- a/Common/Net/DNS/ResolvedIPInformation.cs +++ b/Common/Net/DNS/ResolvedIPInformation.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Concurrent; using System.Net; +using System.Net.Sockets; using System.Threading; using System.Threading.Tasks; @@ -8,11 +9,11 @@ namespace Wokhan.WindowsFirewallNotifier.Common.Net.DNS; -public class ResolvedIPInformation +public class ResolvedIPInformation : IDisposable { internal static readonly ConcurrentDictionary CachedIPHostEntryDict = new(); - private readonly ManualResetEventSlim _eventSlim = new ManualResetEventSlim(true); + private readonly ManualResetEventSlim _eventSlim = new(true); private readonly string ip; @@ -27,10 +28,20 @@ public class ResolvedIPInformation return String.Empty; } - var entry = CachedIPHostEntryDict.GetOrAdd(ip, sourceIp => new ResolvedIPInformation(sourceIp)); + // If using GetHostEntryAsync in WaitOrResolveHost, SocketExceptions are never catched back and a deadlock occurs... + // So we've to initiate the async task ourselves. + return await Task.Run(() => + { + var entry = CachedIPHostEntryDict.GetOrAdd(ip, sourceIp => new ResolvedIPInformation(sourceIp)); - // Ensure that it has been resolved - return await entry.WaitOrResolveHostAsync(); + if (entry.handled) + { + return entry.resolvedHost; + } + + // Ensure that it has been resolved + return entry.WaitOrResolveHost(); + }); } public ResolvedIPInformation(string ip) @@ -38,29 +49,42 @@ public ResolvedIPInformation(string ip) this.ip = ip; } - private async Task WaitOrResolveHostAsync() + private string WaitOrResolveHost() { - _eventSlim.Wait(); - - if (!handled) + try { - _eventSlim.Reset(); + _eventSlim.Wait(); - try + if (!handled) { - resolvedHost = (await Dns.GetHostEntryAsync(ip)).HostName; - } - catch (Exception e) - { - LogHelper.Debug($"Unable to resolve {ip}, message was {e.Message}"); + _eventSlim.Reset(); + + resolvedHost = Dns.GetHostEntry(ip)?.HostName ?? "N/A"; } + } + catch (SocketException se) + { + resolvedHost = se.Message; + } + catch (Exception e) + { + LogHelper.Debug($"Unable to resolve {ip}, message was {e.Message}"); + } + finally + { handled = true; // Releases all pending entry resolutions in other threads for this entry + // TODO: _eventSlim might need to be disposed once the hostname resolution is done and after no thread is waiting... Could induce an unjustified memory use if noe? _eventSlim.Set(); } return resolvedHost; } + + public void Dispose() + { + _eventSlim.Dispose(); + } } diff --git a/Common/Processes/ProcessHelper.cs b/Common/Processes/ProcessHelper.cs index 15ea1c2..15f77b7 100644 --- a/Common/Processes/ProcessHelper.cs +++ b/Common/Processes/ProcessHelper.cs @@ -644,8 +644,13 @@ public static void StartOrRestoreToForeground(ProcessNames processName) /// Opens Windows explorer and selects the file targeted by "flepath" /// /// Full path to the file to select - public static void BrowseToFile(string filepath) + public static void BrowseToFile(string? filepath) { + if (filepath is null) + { + return; + } + StartShellExecutable(ProcessNames.Explorer.FileName, $"/select,{filepath}", true); } @@ -653,8 +658,13 @@ public static void BrowseToFile(string filepath) /// Opens a folder in Windows explorer. /// /// Path to the folder - public static void OpenFolder(string folderPath) + public static void OpenFolder(string? folderPath) { + if (folderPath is null) + { + return; + } + StartShellExecutable(ProcessNames.Explorer.FileName, folderPath, true); } diff --git a/Common/UI/ViewModels/ConnectionBaseInfo.cs b/Common/UI/ViewModels/ConnectionBaseInfo.cs index 7686ad3..86093ed 100644 --- a/Common/UI/ViewModels/ConnectionBaseInfo.cs +++ b/Common/UI/ViewModels/ConnectionBaseInfo.cs @@ -68,7 +68,6 @@ public string? TargetHostName public int RawProtocol { get; protected set; } public string? Protocol { get; protected set; } public string? Direction { get; protected set; } - public string? FilterId { get; protected set; } private bool fileInfoResolutionTriggered; diff --git a/Common/UI/ViewModels/LogEntryViewModel.cs b/Common/UI/ViewModels/LogEntryViewModel.cs index d8bd251..e36018e 100644 --- a/Common/UI/ViewModels/LogEntryViewModel.cs +++ b/Common/UI/ViewModels/LogEntryViewModel.cs @@ -97,7 +97,7 @@ public class LogEntryViewModel : ConnectionBaseInfo return false; } - private static string GetReplacementString(EventLogEntry entry, int i) + private static string? GetReplacementString(EventLogEntry entry, int i) { return entry.ReplacementStrings.DefaultIfEmpty(string.Empty).ElementAtOrDefault(i); } diff --git a/Console/UI/Controls/BandwidthGraph.xaml.cs b/Console/UI/Controls/BandwidthGraph.xaml.cs index 7f373a4..7bb9a1b 100644 --- a/Console/UI/Controls/BandwidthGraph.xaml.cs +++ b/Console/UI/Controls/BandwidthGraph.xaml.cs @@ -107,7 +107,7 @@ private void InitAxes() public double CurrentStart { - get => (double)(xAxis is not null ? xAxis.MinLimit + (xAxis.MaxLimit - xAxis.MinLimit) / 2 : 0); + get => (xAxis is not null ? xAxis.MinLimit + (xAxis.MaxLimit - xAxis.MinLimit) / 2 : 0) ?? 0; set { if (isXPanned) diff --git a/Console/UI/Controls/Map.xaml.cs b/Console/UI/Controls/Map.xaml.cs index ae33234..7ca1ee1 100644 --- a/Console/UI/Controls/Map.xaml.cs +++ b/Console/UI/Controls/Map.xaml.cs @@ -42,14 +42,14 @@ public ObservableCollection Connections public bool IsAerial { get => Mode is AerialMode; - set { Mode = (value ? new AerialMode(true) : (MapMode)new RoadMode()); } + set { Mode = (value ? new AerialMode(true) : new RoadMode()); } } [ObservableProperty] [NotifyPropertyChangedFor(nameof(IsAerial))] private MapMode _mode = new RoadMode(); - public ObservableCollection ConnectionsRoutes { get; } = new ObservableCollection(); + public ObservableCollection ConnectionsRoutes { get; } = new(); public Map() { @@ -102,7 +102,7 @@ async void Map_Loaded(object sender, RoutedEventArgs e) ProgressStack.Visibility = Visibility.Collapsed; } - private void GeoWatcher_PositionChanged(object sender, GeoPositionChangedEventArgs e) + private void GeoWatcher_PositionChanged(object? sender, GeoPositionChangedEventArgs e) { if (!geoWatcher.Position.Location.IsUnknown) { @@ -142,7 +142,7 @@ public void UpdateMap() } //TODO: Temporary check for addresses validity (for mapping purpose only). Doesn't look like the right way to do this. - private bool IsValid(string remoteAddress) + private bool IsValid(string? remoteAddress) { return (!String.IsNullOrEmpty(remoteAddress) && remoteAddress != "127.0.0.1" diff --git a/Console/UI/Pages/AdapterInfo.xaml b/Console/UI/Pages/AdapterInfo.xaml index 66e4c59..73416c9 100644 --- a/Console/UI/Pages/AdapterInfo.xaml +++ b/Console/UI/Pages/AdapterInfo.xaml @@ -96,9 +96,9 @@ - + - + @@ -141,7 +141,7 @@