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

Fix assembly load when file path contains URI reserved chars #76617

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Jan 3, 2025

Resolves microsoft/vscode-dotnettools#1686

When vscode is installed in (or an extension assembly is loaded from) a path that contains a URI reserved character, the server crashes. This is because VS-MEF (for compatibility with clr behavior) sets the code base to a file URI with an unencoded path.

When parsing the URI, some reserved characters cause the path component to be incorrectly split into query or fragment components if unescaped. Then we get an invalid file path from the URI and cannot load the extension dll.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2025
@dibarbet dibarbet force-pushed the fix_load_reserved_char branch from 3077fa8 to 68ed6d9 Compare January 3, 2025 23:03
@dibarbet dibarbet marked this pull request as ready for review January 4, 2025 00:54
@dibarbet dibarbet requested a review from a team as a code owner January 4, 2025 00:54
@dibarbet dibarbet requested a review from JoeRobich January 4, 2025 00:54
@dibarbet dibarbet merged commit 0e54217 into dotnet:main Jan 6, 2025
25 checks passed
@dibarbet dibarbet deleted the fix_load_reserved_char branch January 6, 2025 21:23
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jan 6, 2025
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
var dllData = compilation.EmitToStream();
tempFile.WriteAllBytes(dllData.ToArray());

// Mark the file as read only to prevent mutations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break us cleaning up the temp files later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see issues, but let me check. I stole this from a compiler test

@jasonmalinowski
Copy link
Member

@dibarbet Just generally curious here:

  1. Should VS MEF give some alternative here to avoid us having to work around this behavior? Or a common implementation of IAssemblyLoader that smooths this over? Because I imagine DevKit and other extensions would have the same issue, right?
  2. Are we able to just avoid using the codebase parameter entirely, in some way?

@dibarbet
Copy link
Member Author

dibarbet commented Jan 10, 2025

Should VS MEF give some alternative here to avoid us having to work around this behavior? Or a common implementation of IAssemblyLoader that smooths this over? Because I imagine DevKit and other extensions would have the same issue, right?

Possibly. However we do want our own IASsemblyLoader here so we can load assemblies into matching ALCs. So they would have to provide us with some consistent way to get the file path.

I don't think devkit has an issue here because they don't need the codebase to load it into a directory based ALC (we only need it for our extensions).

Are we able to just avoid using the codebase parameter entirely, in some way?

I do not know of one. As best as I can tell it is the only way to get the file path, which we need to put it in the directory ALC.

@jasonmalinowski
Copy link
Member

However we do want our own IASsemblyLoader here so we can load assemblies into matching ALCs. So they would have to provide us with some consistent way to get the file path.

Oh we absolutely need one. I was thinking that if we can't change the interface, then instead they could make a base class we inherit from, which implements the interface method and adds whatever magic we're putting here into that. That way at least all the codebase shenanigans are in one repo where we don't have to worry about things getting out of sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Could not find assembly Microsoft.VisualStudio.DesignTools.CodeAnalysis
3 participants