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 symlink creation on older Windows versions #13488

Closed
wants to merge 7 commits into from

Conversation

khogeland
Copy link

@khogeland khogeland commented May 18, 2021

This patch ensures that the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag is not passed to CreateSymbolicLinkW when the developer mode registry key is not present and enabled on a supported version of Windows. This allows symlinks to be created when Bazel is run with elevated privileges, regardless of Windows version or developer mode.

I also removed the dummy file creation check while refactoring that code for reuse. It seemed overly complicated vs. simply checking the registry and failing during runfiles creation if we're not admin. Please let me know if there's some subtle reason it needed to be done that way.

Fixes #13169 - tested on Windows Server 2016.

@google-cla
Copy link

google-cla bot commented May 18, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label May 18, 2021
@jin jin added area-Windows Windows-specific issues and feature requests team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website labels May 19, 2021
@meteorcloudy meteorcloudy self-requested a review May 19, 2021 07:40
@meteorcloudy meteorcloudy self-assigned this May 19, 2021
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks! This indeed looks better than the previous check overall.

But one corner case is that: what if you are on a Windows version prior to Windows 10 Creators Update (1703), and you also have developer enabled. In this case, the implementation in this PR will try to create the symlink with the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag, which the system doesn't support. However it should be able to create the symlink if the user has admin right.

Can you please verify if this is a problem on Windows Server 2016?

src/main/tools/BUILD Show resolved Hide resolved
@philwo
Copy link
Member

philwo commented May 19, 2021

I was going to say that Windows 1703 (even Enterprise) is long EOL since 2019, but indeed Windows Server 2016 is still supported until 2022 😳

@khogeland
Copy link
Author

khogeland commented May 19, 2021

Ah, alright, I misinterpreted the Bazel docs and thought those features were introduced at the same time, but that is a possible situation.

@khogeland
Copy link
Author

@meteorcloudy I've added an explicit OS version check to make sure Bazel never passes the flag to an incompatible version of Windows.

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thank you so much! Getting rid of the dummy link approach is really nice!

src/main/tools/BUILD Show resolved Hide resolved
@meteorcloudy
Copy link
Member

@khogeland Can you please sign the CLA so that I can merge this PR? 😃

@khogeland
Copy link
Author

@meteorcloudy I see the cla: yes label, is there something else I need to do? I rewrote my initial commit to use the email on our CCLA

@meteorcloudy
Copy link
Member

@khogeland Oh, my mistake, everything is good to go!

@bazel-io bazel-io closed this in 2f0927a May 21, 2021
bazel-io pushed a commit that referenced this pull request Jul 2, 2021
The OS version check in #13488 breaks the developer mode symlink behavior.

`IsWindowsVersionOrGreater` does not work as advertised, and returns false on Windows 10 if not called from an executable with an associated application manifest declaring its compatibility for Windows 10. (Very cool, Microsoft.) The other methods of checking OS version are far more verbose and complicated, which doesn't seem warranted here.

As an alternative workaround, this PR replaces the ahead-of-time version check with a retry without the flag if the function reports an invalid argument exception.

@meteorcloudy

Closes #13629.

PiperOrigin-RevId: 382734470
katre pushed a commit that referenced this pull request Jul 12, 2021
This patch ensures that the `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` flag is not passed to `CreateSymbolicLinkW` when the developer mode registry key is not present and enabled on a supported version of Windows. This allows symlinks to be created when Bazel is run with elevated privileges, regardless of Windows version or developer mode.

I also removed the dummy file creation check while refactoring that code for reuse. It seemed overly complicated vs. simply checking the registry and failing during runfiles creation if we're not admin. Please let me know if there's some subtle reason it needed to be done that way.

Fixes #13169 - tested on Windows Server 2016.

Closes #13488.

PiperOrigin-RevId: 375035407
@meteorcloudy meteorcloudy mentioned this pull request Jul 13, 2021
9 tasks
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
This patch ensures that the `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` flag is not passed to `CreateSymbolicLinkW` when the developer mode registry key is not present and enabled on a supported version of Windows. This allows symlinks to be created when Bazel is run with elevated privileges, regardless of Windows version or developer mode.

I also removed the dummy file creation check while refactoring that code for reuse. It seemed overly complicated vs. simply checking the registry and failing during runfiles creation if we're not admin. Please let me know if there's some subtle reason it needed to be done that way.

Fixes bazelbuild#13169 - tested on Windows Server 2016.

Closes bazelbuild#13488.

PiperOrigin-RevId: 375035407
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
The OS version check in bazelbuild#13488 breaks the developer mode symlink behavior.

`IsWindowsVersionOrGreater` does not work as advertised, and returns false on Windows 10 if not called from an executable with an associated application manifest declaring its compatibility for Windows 10. (Very cool, Microsoft.) The other methods of checking OS version are far more verbose and complicated, which doesn't seem warranted here.

As an alternative workaround, this PR replaces the ahead-of-time version check with a retry without the flag if the function reports an invalid argument exception.

@meteorcloudy

Closes bazelbuild#13629.

PiperOrigin-RevId: 382734470
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
This patch ensures that the `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` flag is not passed to `CreateSymbolicLinkW` when the developer mode registry key is not present and enabled on a supported version of Windows. This allows symlinks to be created when Bazel is run with elevated privileges, regardless of Windows version or developer mode.

I also removed the dummy file creation check while refactoring that code for reuse. It seemed overly complicated vs. simply checking the registry and failing during runfiles creation if we're not admin. Please let me know if there's some subtle reason it needed to be done that way.

Fixes bazelbuild#13169 - tested on Windows Server 2016.

Closes bazelbuild#13488.

PiperOrigin-RevId: 375035407
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
The OS version check in bazelbuild#13488 breaks the developer mode symlink behavior.

`IsWindowsVersionOrGreater` does not work as advertised, and returns false on Windows 10 if not called from an executable with an associated application manifest declaring its compatibility for Windows 10. (Very cool, Microsoft.) The other methods of checking OS version are far more verbose and complicated, which doesn't seem warranted here.

As an alternative workaround, this PR replaces the ahead-of-time version check with a retry without the flag if the function reports an invalid argument exception.

@meteorcloudy

Closes bazelbuild#13629.

PiperOrigin-RevId: 382734470
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel fails to create symlinks on slightly older versions of Windows
4 participants