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

Inconsistent path handling results in miscompilation on Windows systems #170767

Closed
xaberus opened this issue Jan 7, 2023 · 6 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug engineering VS Code - Build / issue tracking / etc. verified Verification succeeded
Milestone

Comments

@xaberus
Copy link

xaberus commented Jan 7, 2023

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.74.0+
  • OS Version: Windows 10

Steps to Reproduce:

  1. yarn --frozen-lockfile
  2. yarn gulp compile-build

On Windows systems the data.path field in

const newContents = newContentsByFileName.get(data.path);

is \ separated but the TS compiler that generates the newContentsByFileName map uses / slashes. Therefore none of the replacements done by the mangler are found.

In the context of the remote extension hosts this leads to problems when the Windows built Windows client connects to, i.e., a Linux built Linux client. Names of private fields leak in the JSON.stringify() serialization in

protocol.send(VSBuffer.fromString(JSON.stringify(data)));

and cause surprising bugs. (These bugs are VSCode-OSS specific even though they are filled against VSCodium).

A possible solution can be found here: VSCodium/vscodium#1370

@VSCodeTriageBot
Copy link
Collaborator

Thanks for creating this issue! It looks like you may be using an old version of VS Code, the latest stable release is 1.74.2. Please try upgrading to the latest version and checking whether this issue remains.

Happy Coding!

@xaberus
Copy link
Author

xaberus commented Jan 7, 2023

The issue exists in all releases newer than 1.73.1.

@lramos15 lramos15 assigned mjbvz and unassigned lramos15 Jan 20, 2023
@jrieken jrieken added this to the January 2023 milestone Jan 23, 2023
@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug engineering VS Code - Build / issue tracking / etc. labels Jan 23, 2023
@jrieken
Copy link
Member

jrieken commented Jan 23, 2023

Thanks. This is a great find. At first I will fix the pipeline scripts, second we will think about the fact that serialization depends on private object state

@jrieken
Copy link
Member

jrieken commented Jan 23, 2023

fyi @alexdima This is about ExtensionIdentifier and potentially others. The _lower property will be mangled at build times and if that happens unequally* the toKey function fails.

* As of today this bug is the only case where unequal mangling happens, the names are generated in a deterministic fashion and for types that aren't involved in inheritance things are simple. Tho, we might wanna implement toJSON explicitly or mark the field as public

jrieken added a commit that referenced this issue Jan 23, 2023
* use normalized path when checking for mangled new contents,

#170767

* add missing js file
@jrieken jrieken modified the milestones: January 2023, February 2023 Jan 25, 2023
jeanp413 added a commit to jeanp413/vscode that referenced this issue Jan 29, 2023
@jeanp413
Copy link
Contributor

@jrieken created a PR #172739 fixing the bug reported in this comment

@jrieken jrieken closed this as completed Jan 30, 2023
@jrieken
Copy link
Member

jrieken commented Jan 30, 2023

Thanks @jeanp413

@connor4312 connor4312 added the verified Verification succeeded label Feb 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug engineering VS Code - Build / issue tracking / etc. verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants