Sanitize Reg Entries in SerialPort.GetPortNames (Issue 93240) #93242
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFix for Issue 93240
|
|
Tagging subscribers to this area: @dotnet/area-system-io-ports Issue DetailsFix for Issue 93240
|
There was a problem hiding this comment.
@ctacke thank you for your contribution!
The main owner of System.IO.Ports is currently offline, in the meantime I've tagged the owners of Microsoft.Win32.Registry to verify whether it should be fixed here or in the API that IO Ports is simply calling.
| result[i] = (string)serialKey.GetValue(result[i]); | ||
| // Replace the name in the array with its value, trimming at the first null character if it exists | ||
| var temp = (string)serialKey.GetValue(result[i]); | ||
| var end = temp.IndexOf('\0'); |
There was a problem hiding this comment.
I am not sure whether we should try to workaround the null characters here. Ideally RegistryKey.GetValueNames should do it out of the box.
@dotnet/area-microsoft-win32 thoughts?
There was a problem hiding this comment.
GetValueNames looks like better choice to me. can you please give it try @ctacke ?
There was a problem hiding this comment.
I'm not sure what the suggestion is. The Value Name is not the issue, the Value itself is. I can't "try GetValueNames" as a resolution, because it doesn't give the data I need.
You could certainly argue that Registry.GetValue should be stopping at the first null, but I don't know if there are other instances where a null-containing string might be valid data. If null-containing strings are generally considered invalid data for registry entries, then yes, the bug would be in Registry.GetValue
There was a problem hiding this comment.
I may be wrong but it seems like this is what the method does e.g. terminates on null (while the GetValue does not)
https://learn.microsoft.com/en-us/dotnet/api/microsoft.win32.registrykey.getvaluenames?view=net-8.0#microsoft-win32-registrykey-getvaluenames
Perhaps @adamsitnik can explain more.
|
@ctacke could you please investigate if the recommended API is suitable for usage? I don't have any hardware where I could try this out |
krwq
added
the
needs-author-action
|
This pull request has been automatically marked |
|
This pull request will now be closed since it had been marked |
ghost
closed this
Fixes: #93240
Edit (@krwq): Using correct syntax so that issue linking works