From c78413cf7cee018d50bce8e02d9cb7e3e5626d90 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 6 Apr 2020 00:03:56 -0400 Subject: [PATCH 1/5] Disable UPnP by default --- MediaBrowser.Model/Configuration/ServerConfiguration.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MediaBrowser.Model/Configuration/ServerConfiguration.cs b/MediaBrowser.Model/Configuration/ServerConfiguration.cs index 3107ec2426..9983aa0e09 100644 --- a/MediaBrowser.Model/Configuration/ServerConfiguration.cs +++ b/MediaBrowser.Model/Configuration/ServerConfiguration.cs @@ -254,7 +254,7 @@ namespace MediaBrowser.Model.Configuration AutoRunWebApp = true; EnableRemoteAccess = true; - EnableUPnP = true; + EnableUPnP = false; MinResumePct = 5; MaxResumePct = 90; From 26afb42a72ec111675e079be7bdee13ea05c9713 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 6 Apr 2020 00:05:47 -0400 Subject: [PATCH 2/5] Cleanup port forwarding service - Use a concurrent collection instead of manually locking - Do not forward HTTPS port when it is not enabled - Created multiple rules (HTTP/HTTPS) in parallel instead of in sync --- .../EntryPoints/ExternalPortForwarding.cs | 79 ++++++++----------- 1 file changed, 33 insertions(+), 46 deletions(-) diff --git a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs index e290c62e16..5525401c5e 100644 --- a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs +++ b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs @@ -1,6 +1,7 @@ #pragma warning disable CS1591 using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Net; using System.Text; @@ -26,8 +27,8 @@ namespace Emby.Server.Implementations.EntryPoints private readonly IServerConfigurationManager _config; private readonly IDeviceDiscovery _deviceDiscovery; - private readonly object _createdRulesLock = new object(); - private List _createdRules = new List(); + private readonly ConcurrentDictionary _createdRules = new ConcurrentDictionary(); + private Timer _timer; private string _lastConfigIdentifier; @@ -98,7 +99,7 @@ namespace Emby.Server.Implementations.EntryPoints NatUtility.DeviceFound += OnNatUtilityDeviceFound; NatUtility.StartDiscovery(); - _timer = new Timer(ClearCreatedRules, null, TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10)); + _timer = new Timer((_) => _createdRules.Clear(), null, TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10)); _deviceDiscovery.DeviceDiscovered += OnDeviceDiscoveryDeviceDiscovered; @@ -117,26 +118,16 @@ namespace Emby.Server.Implementations.EntryPoints _deviceDiscovery.DeviceDiscovered -= OnDeviceDiscoveryDeviceDiscovered; } - private void ClearCreatedRules(object state) - { - lock (_createdRulesLock) - { - _createdRules.Clear(); - } - } - private void OnDeviceDiscoveryDeviceDiscovered(object sender, GenericEventArgs e) { NatUtility.Search(e.Argument.LocalIpAddress, NatProtocol.Upnp); } - private void OnNatUtilityDeviceFound(object sender, DeviceEventArgs e) + private async void OnNatUtilityDeviceFound(object sender, DeviceEventArgs e) { try { - var device = e.Device; - - CreateRules(device); + await CreateRules(e.Device).ConfigureAwait(false); } catch (Exception ex) { @@ -144,7 +135,7 @@ namespace Emby.Server.Implementations.EntryPoints } } - private async void CreateRules(INatDevice device) + private Task CreateRules(INatDevice device) { if (_disposed) { @@ -153,50 +144,46 @@ namespace Emby.Server.Implementations.EntryPoints // On some systems the device discovered event seems to fire repeatedly // This check will help ensure we're not trying to port map the same device over and over - var address = device.DeviceEndpoint; - - lock (_createdRulesLock) + if (!_createdRules.TryAdd(device.DeviceEndpoint, 0)) { - if (!_createdRules.Contains(address)) - { - _createdRules.Add(address); - } - else - { - return; - } + return Task.CompletedTask; } - try - { - await CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort).ConfigureAwait(false); - } - catch (Exception ex) - { - _logger.LogError(ex, "Error creating http port map"); - return; - } + return Task.WhenAll(CreatePortMaps(device)); + } - try + private IEnumerable CreatePortMaps(INatDevice device) + { + yield return CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort); + + if (_appHost.EnableHttps) { - await CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort).ConfigureAwait(false); - } - catch (Exception ex) - { - _logger.LogError(ex, "Error creating https port map"); + yield return CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort); } } - private Task CreatePortMap(INatDevice device, int privatePort, int publicPort) + private async Task CreatePortMap(INatDevice device, int privatePort, int publicPort) { _logger.LogDebug( - "Creating port map on local port {0} to public port {1} with device {2}", + "Creating port map on local port {LocalPort} to public port {PublicPort} with device {DeviceEndpoint}", privatePort, publicPort, device.DeviceEndpoint); - return device.CreatePortMapAsync( - new Mapping(Protocol.Tcp, privatePort, publicPort, 0, _appHost.Name)); + try + { + var mapping = new Mapping(Protocol.Tcp, privatePort, publicPort, 0, _appHost.Name); + await device.CreatePortMapAsync(mapping).ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.LogError( + ex, + "Error creating port map on local port {LocalPort} to public port {PublicPort} with device {DeviceEndpoint}.", + privatePort, + publicPort, + device.DeviceEndpoint); + } } /// From 78d9b9894c5ad9478852196213b8585c37456128 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 6 Apr 2020 00:22:27 -0400 Subject: [PATCH 3/5] Respond to config changes correctly for external port forwarding --- .../EntryPoints/ExternalPortForwarding.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs index 5525401c5e..8dbc6494d5 100644 --- a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs +++ b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs @@ -30,7 +30,7 @@ namespace Emby.Server.Implementations.EntryPoints private readonly ConcurrentDictionary _createdRules = new ConcurrentDictionary(); private Timer _timer; - private string _lastConfigIdentifier; + private string _configIdentifier; private bool _disposed = false; @@ -70,7 +70,10 @@ namespace Emby.Server.Implementations.EntryPoints private void OnConfigurationUpdated(object sender, EventArgs e) { - if (!string.Equals(_lastConfigIdentifier, GetConfigIdentifier(), StringComparison.OrdinalIgnoreCase)) + var oldConfigIdentifier = _configIdentifier; + _configIdentifier = GetConfigIdentifier(); + + if (!string.Equals(_configIdentifier, oldConfigIdentifier, StringComparison.OrdinalIgnoreCase)) { Stop(); Start(); @@ -94,7 +97,7 @@ namespace Emby.Server.Implementations.EntryPoints return; } - _logger.LogDebug("Starting NAT discovery"); + _logger.LogInformation("Starting NAT discovery"); NatUtility.DeviceFound += OnNatUtilityDeviceFound; NatUtility.StartDiscovery(); @@ -102,13 +105,11 @@ namespace Emby.Server.Implementations.EntryPoints _timer = new Timer((_) => _createdRules.Clear(), null, TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10)); _deviceDiscovery.DeviceDiscovered += OnDeviceDiscoveryDeviceDiscovered; - - _lastConfigIdentifier = GetConfigIdentifier(); } private void Stop() { - _logger.LogDebug("Stopping NAT discovery"); + _logger.LogInformation("Stopping NAT discovery"); NatUtility.StopDiscovery(); NatUtility.DeviceFound -= OnNatUtilityDeviceFound; From 8a81bcd7425ebf93eb688bbc754179dfc20787ec Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Thu, 16 Apr 2020 22:49:23 -0400 Subject: [PATCH 4/5] Restart port forwarding when public https port changes --- .../EntryPoints/ExternalPortForwarding.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs index 8dbc6494d5..37d7fd4799 100644 --- a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs +++ b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs @@ -61,6 +61,7 @@ namespace Emby.Server.Implementations.EntryPoints return new StringBuilder(32) .Append(config.EnableUPnP).Append(Separator) .Append(config.PublicPort).Append(Separator) + .Append(config.PublicHttpsPort).Append(Separator) .Append(_appHost.HttpPort).Append(Separator) .Append(_appHost.HttpsPort).Append(Separator) .Append(_appHost.EnableHttps).Append(Separator) From bcf1ef319a228dc6383b1730e8222c1b2294e697 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Fri, 17 Apr 2020 00:27:18 -0400 Subject: [PATCH 5/5] Update documentation for EnableUPnP --- MediaBrowser.Model/Configuration/ServerConfiguration.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/MediaBrowser.Model/Configuration/ServerConfiguration.cs b/MediaBrowser.Model/Configuration/ServerConfiguration.cs index 9983aa0e09..b5e8d5589a 100644 --- a/MediaBrowser.Model/Configuration/ServerConfiguration.cs +++ b/MediaBrowser.Model/Configuration/ServerConfiguration.cs @@ -15,9 +15,8 @@ namespace MediaBrowser.Model.Configuration private string _baseUrl; /// - /// Gets or sets a value indicating whether [enable u pn p]. + /// Gets or sets a value indicating whether to enable automatic port forwarding. /// - /// true if [enable u pn p]; otherwise, false. public bool EnableUPnP { get; set; } ///