From bc752b89a4774cc80a666bdb045414c4d27cbdec Mon Sep 17 00:00:00 2001 From: Frans van Dorsselaer <17404029+dorssel@users.noreply.github.com> Date: Sun, 31 Dec 2023 23:21:52 +0100 Subject: [PATCH] Fix crash for incompatible hubs --- UnitTests/BusId_Tests.cs | 79 +++++++++++++++++++------------ Usbipd.Automation/BusId.cs | 55 ++++++++++++++++----- Usbipd.PowerShell/Installation.cs | 2 +- Usbipd/CommandHandlers.cs | 29 ++++++++---- Usbipd/ConfigurationManager.cs | 15 +++--- Usbipd/Program.cs | 30 ++++++------ 6 files changed, 137 insertions(+), 73 deletions(-) diff --git a/UnitTests/BusId_Tests.cs b/UnitTests/BusId_Tests.cs index 2784a3bc..e9467666 100644 --- a/UnitTests/BusId_Tests.cs +++ b/UnitTests/BusId_Tests.cs @@ -16,6 +16,25 @@ public void DefaultConstructor() Assert.AreEqual(0, busId.Bus); Assert.AreEqual(0, busId.Port); + Assert.IsTrue(busId.IsIncompatibleHub); + } + + [TestMethod] + public void ConstructorWithInvalidBusThrows() + { + Assert.ThrowsException(() => + { + var busId = new BusId(0, 1); + }); + } + + [TestMethod] + public void ConstructorWithInvalidPortThrows() + { + Assert.ThrowsException(() => + { + var busId = new BusId(1, 0); + }); } [TestMethod] @@ -28,6 +47,15 @@ public void JsonConstructor() Assert.AreEqual(testBus, busId.Bus); Assert.AreEqual(testPort, busId.Port); + Assert.IsFalse(busId.IsIncompatibleHub); + } + + [TestMethod] + public void ÍncompatibleHub() + { + Assert.AreEqual(0, BusId.IncompatibleHub.Bus); + Assert.AreEqual(0, BusId.IncompatibleHub.Port); + Assert.IsTrue(BusId.IncompatibleHub.IsIncompatibleHub); } sealed class BusIdData @@ -43,10 +71,13 @@ sealed class BusIdData "1-1 ", "a-1", "1-a", + "0-0", "0-1", "1-0", "1-65536", "65536-1", + "01-1", + "1-01", ]; public static IEnumerable Invalid @@ -55,6 +86,7 @@ public static IEnumerable Invalid } static readonly string[] _Valid = [ + "IncompatibleHub", "1-1", "1-2", "1-65534", @@ -73,31 +105,22 @@ public static IEnumerable Invalid "65535-65535", ]; - public static IEnumerable Valid - { - get => from value in _Valid select new string[] { value }; - } + public static IEnumerable Valid => from value in _Valid select new string[] { value }; - static int ExpectedCompare(string left, string right) - { - var leftBus = ushort.Parse(left.Split('-')[0]); - var leftPort = ushort.Parse(left.Split('-')[1]); - var rightBus = ushort.Parse(right.Split('-')[0]); - var rightPort = ushort.Parse(right.Split('-')[1]); - - return - leftBus < rightBus ? -1 : - leftBus > rightBus ? 1 : - leftPort < rightPort ? -1 : - leftPort > rightPort ? 1 : 0; - } + static int ExpectedCompare(string left, string right) => + BusFromValidBusId(left) < BusFromValidBusId(right) ? -1 : + BusFromValidBusId(left) > BusFromValidBusId(right) ? 1 : + PortFromValidBusId(left) < PortFromValidBusId(right) ? -1 : + PortFromValidBusId(left) > PortFromValidBusId(right) ? 1 : 0; - public static IEnumerable Compare - { - get => from left in _Valid from right in _Valid select new object[] { left, right, ExpectedCompare(left, right) }; - } + public static IEnumerable Compare => from left in _Valid from right in _Valid select new object[] { left, right, ExpectedCompare(left, right) }; } + static ushort BusFromValidBusId(string text) => (text == "IncompatibleHub") ? (ushort)0 : ushort.Parse(text.Split('-')[0]); + + static ushort PortFromValidBusId(string text) => (text == "IncompatibleHub") ? (ushort)0 : ushort.Parse(text.Split('-')[1]); + + [TestMethod] [DynamicData(nameof(BusIdData.Invalid), typeof(BusIdData))] public void TryParseInvalid(string text) @@ -106,6 +129,7 @@ public void TryParseInvalid(string text) Assert.IsFalse(result); Assert.AreEqual(0, busId.Bus); Assert.AreEqual(0, busId.Port); + Assert.IsTrue(busId.IsIncompatibleHub); } [TestMethod] @@ -114,11 +138,8 @@ public void TryParseValid(string text) { var result = BusId.TryParse(text, out var busId); Assert.IsTrue(result); - - var expectedBus = ushort.Parse(text.Split('-')[0]); - var expectedPort = ushort.Parse(text.Split('-')[1]); - Assert.AreEqual(expectedBus, busId.Bus); - Assert.AreEqual(expectedPort, busId.Port); + Assert.AreEqual(BusFromValidBusId(text), busId.Bus); + Assert.AreEqual(PortFromValidBusId(text), busId.Port); } [TestMethod] @@ -136,10 +157,8 @@ public void ParseInvalid(string text) public void ParseValid(string text) { var busId = BusId.Parse(text); - var expectedBus = ushort.Parse(text.Split('-')[0]); - var expectedPort = ushort.Parse(text.Split('-')[1]); - Assert.AreEqual(expectedBus, busId.Bus); - Assert.AreEqual(expectedPort, busId.Port); + Assert.AreEqual(BusFromValidBusId(text), busId.Bus); + Assert.AreEqual(PortFromValidBusId(text), busId.Port); } [TestMethod] diff --git a/Usbipd.Automation/BusId.cs b/Usbipd.Automation/BusId.cs index 11d70faa..5f09038d 100644 --- a/Usbipd.Automation/BusId.cs +++ b/Usbipd.Automation/BusId.cs @@ -14,32 +14,61 @@ public readonly record struct BusId { #if !NETSTANDARD [JsonConstructor] - public BusId(ushort bus, ushort port) => (Bus, Port) = (bus, port); #endif + public BusId(ushort bus, ushort port) + { + // Do not allow the explicit creation of the special IncompatibleHub value. + // Instead, use the static IncompatibleHub field (preferrable) or "default". + if (bus == 0) + { + throw new ArgumentOutOfRangeException(nameof(bus)); + } + if (port == 0) + { + throw new ArgumentOutOfRangeException(nameof(port)); + } + Bus = bus; + Port = port; + } - public ushort Bus { get; init; } - public ushort Port { get; init; } + public ushort Bus { get; } + public ushort Port { get; } - public override readonly string ToString() => $"{Bus}-{Port}"; + /// + /// The special value 0-0 for hubs that do not expose a compatible busid. + /// NOTE: This is also the "default" value for this type. + /// + public static readonly BusId IncompatibleHub; +#if !NETSTANDARD + [JsonIgnore] +#endif + public bool IsIncompatibleHub => (Bus == 0) || (Port == 0); + + public override readonly string ToString() => IsIncompatibleHub ? nameof(IncompatibleHub) : $"{Bus}-{Port}"; + + /// + /// NOTE: Valid inputs are x-y, where either x and y are between 1 and 65535, or both are 0. + /// NOTE: We do not allow leading zeros on non-zero values. + /// public static bool TryParse(string input, out BusId busId) { - // Must be 'x-y', where x and y are positive integers without leading zeros. + if (input == nameof(IncompatibleHub)) + { + busId = IncompatibleHub; + return true; + } var match = Regex.Match(input, "^([1-9][0-9]*)-([1-9][0-9]*)$"); if (match.Success - && ushort.TryParse(match.Groups[1].Value, out var bus) && bus != 0 - && ushort.TryParse(match.Groups[2].Value, out var port) && port != 0) + && ushort.TryParse(match.Groups[1].Value, out var bus) + && ushort.TryParse(match.Groups[2].Value, out var port)) { - busId = new() - { - Bus = bus, - Port = port, - }; + busId = new(bus, port); return true; } else { - busId = default; + busId = IncompatibleHub; return false; } } diff --git a/Usbipd.PowerShell/Installation.cs b/Usbipd.PowerShell/Installation.cs index d99725a4..7edfe943 100644 --- a/Usbipd.PowerShell/Installation.cs +++ b/Usbipd.PowerShell/Installation.cs @@ -81,7 +81,7 @@ public static string ExePath throw new ApplicationFailedException($"PowerShell module version {GitVersionInformation.MajorMinorPatch} does not match installed usbipd-win version {version}."); } #if DEBUG - return Path.GetFullPath(Path.Combine(Path.GetDirectoryName(System.Reflection.Assembly.GetCallingAssembly().Location), @"..\..\..\..\..\Usbipd\bin\x64\Debug\net7.0-windows8.0\win-x64\usbipd.exe")); + return Path.GetFullPath(Path.Combine(Path.GetDirectoryName(System.Reflection.Assembly.GetCallingAssembly().Location), @"..\..\..\..\Usbipd\bin\x64\Debug\net8.0-windows10.0.17763\win-x64\usbipd.exe")); #else return exeFile; #endif diff --git a/Usbipd/CommandHandlers.cs b/Usbipd/CommandHandlers.cs index 834dad68..4a645e61 100644 --- a/Usbipd/CommandHandlers.cs +++ b/Usbipd/CommandHandlers.cs @@ -36,32 +36,39 @@ interface ICommandHandlers sealed class CommandHandlers : ICommandHandlers { - static IEnumerable GetDevicesByHardwareId(VidPid vidPid, bool connectedOnly, IConsole console) + static List GetDevicesByHardwareId(VidPid vidPid, bool connectedOnly, IConsole console) { if (!CheckNoStub(vidPid, console)) { return []; } + var filtered = new List(); var devices = UsbDevice.GetAll().Where(d => (d.HardwareId == vidPid) && (!connectedOnly || d.BusId.HasValue)); - var found = false; foreach (var device in devices) { if (device.BusId.HasValue) { - found = true; - console.ReportInfo($"Device with hardware-id '{vidPid}' found at busid '{device.BusId.Value}'."); + if (device.BusId.Value.IsIncompatibleHub) + { + console.ReportWarning($"Ignoring device with hardware-id '{vidPid}' connected to an incompatible hub."); + } + else + { + console.ReportInfo($"Device with hardware-id '{vidPid}' found at busid '{device.BusId}'."); + filtered.Add(device); + } } else if (device.Guid.HasValue) { - found = true; console.ReportInfo($"Persisted device with hardware-id '{vidPid}' found at guid '{device.Guid.Value:D}'."); + filtered.Add(device); } } - if (!found) + if (filtered.Count == 0) { console.ReportError($"No devices found with hardware-id '{vidPid}'."); } - return devices; + return filtered; } static BusId? GetBusIdByHardwareId(VidPid vidPid, IConsole console) @@ -148,12 +155,16 @@ Task ICommandHandlers.List(bool usbids, IConsole console, Cancellation { state = device.IsForced ? "Shared (forced)" : "Shared"; } + else if (device.BusId.Value.IsIncompatibleHub) + { + state = "Incompatible hub"; + } else { state = "Not shared"; } // NOTE: Strictly speaking, both Bus and Port can be > 99. If you have one of those, you win a prize! - console.Write($"{device.BusId.Value,-5} "); + console.Write($"{(device.BusId.Value.IsIncompatibleHub ? string.Empty : device.BusId.Value),-5} "); console.Write($"{device.HardwareId,-9} "); console.WriteTruncated(GetDescription(device, usbids), 60, true); console.WriteLine($" {state}"); @@ -498,7 +509,7 @@ Task ICommandHandlers.Detach(BusId busId, IConsole console, Cancellati Task ICommandHandlers.Detach(VidPid vidPid, IConsole console, CancellationToken cancellationToken) { var devices = GetDevicesByHardwareId(vidPid, true, console); - if (!devices.Any()) + if (devices.Count == 0) { // This would result in a no-op, which may not be what the user intended. return Task.FromResult(ExitCode.Failure); diff --git a/Usbipd/ConfigurationManager.cs b/Usbipd/ConfigurationManager.cs index 5f9766ae..f38185b7 100644 --- a/Usbipd/ConfigurationManager.cs +++ b/Usbipd/ConfigurationManager.cs @@ -152,14 +152,17 @@ public static BusId GetBusId(uint deviceNode) var match = LocationInfoRegex().Match(locationInfo); if (!match.Success) { - // We want users to report this one. - throw new NotSupportedException($"DEVPKEY_Device_LocationInfo returned '{locationInfo}', expected form 'Port_#0123.Hub_#4567'"); + // This is probably a device on an unsupported hub-type. + // See for example https://github.com/dorssel/usbipd-win/issues/809. + return BusId.IncompatibleHub; } - return new() + var bus = ushort.Parse(match.Groups[2].Value, CultureInfo.InvariantCulture); + var port = ushort.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture); + if (bus == 0 || port == 0) { - Bus = ushort.Parse(match.Groups[2].Value, CultureInfo.InvariantCulture), - Port = ushort.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture), - }; + return BusId.IncompatibleHub; + } + return new(bus, port); } public static BusId? GetBusId(string instanceId) diff --git a/Usbipd/Program.cs b/Usbipd/Program.cs index 5581f9de..e268823d 100644 --- a/Usbipd/Program.cs +++ b/Usbipd/Program.cs @@ -31,9 +31,9 @@ public enum ExitCode Canceled = 4, }; - static BusId ParseBusId(ArgumentResult argumentResult) + static BusId ParseCompatibleBusId(ArgumentResult argumentResult) { - if (!BusId.TryParse(argumentResult.Tokens[0].Value, out var busId)) + if (!BusId.TryParse(argumentResult.Tokens[0].Value, out var busId) || busId.IsIncompatibleHub) { argumentResult.ErrorMessage = LocalizationResources.Instance.ArgumentConversionCannotParseForOption(argumentResult.Tokens[0].Value, (argumentResult.Parent as OptionResult)?.Token?.Value ?? string.Empty, typeof(BusId)); @@ -96,6 +96,12 @@ internal static IEnumerable CompletionGuard(CompletionContext completion } } + internal static IEnumerable CompatibleBusIdCompletions(CompletionContext completionContext) + { + return CompletionGuard(completionContext, () => + UsbDevice.GetAll().Where(d => d.BusId.HasValue && !d.BusId.Value.IsIncompatibleHub).Select(d => d.BusId.GetValueOrDefault().ToString())); + } + internal static int Main(params string[] args) { if (!Console.IsInputRedirected) @@ -139,13 +145,12 @@ internal static ExitCode Run(IConsole? optionalTestConsole, ICommandHandlers com // var busIdOption = new Option( aliases: ["--busid", "-b"], - parseArgument: ParseBusId + parseArgument: ParseCompatibleBusId ) { ArgumentHelpName = "BUSID", Description = "Attach device having ", - }.AddCompletions(completionContext => CompletionGuard(completionContext, () => - UsbDevice.GetAll().Where(d => d.BusId.HasValue).Select(d => d.BusId.GetValueOrDefault().ToString()))); + }.AddCompletions(CompatibleBusIdCompletions); // // attach --wsl [] // @@ -218,13 +223,12 @@ await commandHandlers.AttachWsl(invocationContext.ParseResult.GetValueForOption( // var busIdOption = new Option( aliases: ["--busid", "-b"], - parseArgument: ParseBusId + parseArgument: ParseCompatibleBusId ) { ArgumentHelpName = "BUSID", Description = "Share device having ", - }.AddCompletions(completionContext => CompletionGuard(completionContext, () => - UsbDevice.GetAll().Where(d => d.BusId.HasValue).Select(d => d.BusId.GetValueOrDefault().ToString()))); + }.AddCompletions(CompatibleBusIdCompletions); // // bind [--force] // @@ -302,13 +306,12 @@ await commandHandlers.Bind(invocationContext.ParseResult.GetValueForOption(hardw // var busIdOption = new Option( aliases: ["--busid", "-b"], - parseArgument: ParseBusId + parseArgument: ParseCompatibleBusId ) { ArgumentHelpName = "BUSID", Description = "Detach device having ", - }.AddCompletions(completionContext => CompletionGuard(completionContext, () => - UsbDevice.GetAll().Where(d => d.BusId.HasValue).Select(d => d.BusId.GetValueOrDefault().ToString()))); + }.AddCompletions(CompatibleBusIdCompletions); // // detach [--hardware-id :] // @@ -461,13 +464,12 @@ await commandHandlers.Server(invocationContext.ParseResult.GetValueForArgument(k // var busIdOption = new Option( aliases: ["--busid", "-b"], - parseArgument: ParseBusId + parseArgument: ParseCompatibleBusId ) { ArgumentHelpName = "BUSID", Description = "Stop sharing device having ", - }.AddCompletions(completionContext => CompletionGuard(completionContext, () => - UsbDevice.GetAll().Where(d => d.BusId.HasValue).Select(d => d.BusId.GetValueOrDefault().ToString()))); + }.AddCompletions(CompatibleBusIdCompletions); // // unbind [--guid ] //