Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows.Win32.System.RemoteDesktop.IWTSVirtualChannelManager.CreateListener : pszChannelName should be a null terminated string instead of Byte* #1272

Merged
merged 3 commits into from Oct 5, 2022

Conversation

LeonarddeR
Copy link
Contributor

Fixes #1185

Note, I'm slightly puzzled because the parameter is const char* in the source, therefore I'd expected this being emitted as PSTR already without this explicit intervention.

…stener : pszChannelName should be a null terminated string instead of Byte*
@mikebattista
Copy link
Contributor

@sotteson1 / @chenss3 could you please review to understand if the tooling is behaving as expected here before we approve the manual override?

@sotteson1
Copy link
Contributor

@sotteson1 / @chenss3 could you please review to understand if the tooling is behaving as expected here before we approve the manual override?

The problem is in the original definition, it explicitly uses const unsigned char * instead of a type we recognize as being a string (like PSTR or LPCSTR). So, it's expected that we would need to add more data to the emitter.

@LeonarddeR
Copy link
Contributor Author

Should I close this pr and create a new one that patches the emitter to cover these cases?

@LeonarddeR
Copy link
Contributor Author

I had a quick look at the tsvirtualchannels.h header in generation/WinSDK/RecompiledIdlHeaders/um and indeed, there the definition is _In_ const unsigned char *pszChannelName,. However, in tsvirtualchannels.idl, the definition is [in, string] const char *pszChannelName,. Also in tsvirtualchannels.h in windows SDK 10.0.22621.0, the unsignedness is missing.

@mikebattista
Copy link
Contributor

Is there an infrastructure issue here or is "patching the emitter" just what you originally did by updating the emitter settings?

@LeonarddeR
Copy link
Contributor Author

I tried updating the emitter by adding "const unsigned char *" to the several cases that emit a pstr, but that didn't changeanything, interestingly.

@riverar
Copy link
Collaborator

riverar commented Oct 1, 2022

[...] So, it's expected that we would need to add more data to the emitter.

I believe @sotteson1's point was that it is expected to need to feed in a more precise type in as this PR is doing. I don't believe he was suggesting that anyone go try to modify the emitter.

That said, we could, as part of a separate task, do something with the __RPC__in_string annotation.

riverar
riverar previously approved these changes Oct 2, 2022
@BenJKuhn BenJKuhn requested review from BenJKuhn and removed request for sotteson1 and BenJKuhn October 3, 2022 18:27
@BenJKuhn
Copy link
Member

BenJKuhn commented Oct 3, 2022

Sorry @sotteson1, I seem to have accidentally removed you from reviewers somehow when adding & removing myself to test something, and github won't let me add you back. Feel free to add yourself again.

@LeonarddeR
Copy link
Contributor Author

I'm slightly puzzled, when I merge main into this branch, I don't get a merge conflict for scripts/ChangesSinceLastRelease.txt and somehow unrelated lines end up in the diff. So I reverted the merge of main for now.

@LeonarddeR
Copy link
Contributor Author

Ah, it instantly became obvious to me that ChangesSinceLastRelease.txt was entirely rebuild since we had a release since my initial commit.

@mikebattista mikebattista merged commit f1307e1 into microsoft:main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IWTSVirtualChannelManager.CreateListener expects a string of type &u8, which might be a regression
6 participants