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 linker error when creating symlinks on Windows #3444

Merged
merged 3 commits into from
Jan 21, 2020
Merged

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Jan 8, 2020

When working on Windows builds for ponyup, I found that it wouldn't
link due to an error with the FFI call in symlink. This commit
adds a test to verify that symlink works.

This PR contains a test that I verified fails on Windows and then adds a change to FilePath.symlink that then allows the failing test to pass.

When working on Windows builds for ponyup, I found that it wouldn't
link due to an error with the FFI call in `symlink`. This commit
adds a test to verify that `symlink` works.

I expect this to fail CI on Windows at which point I will add a fix
and verify that CI then passes on Windows.
@SeanTAllen SeanTAllen changed the title Add FilePath.symlink test Fix linker error when creating symlinks on Windows Jan 8, 2020
@SeanTAllen SeanTAllen added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge do not merge This PR should not be merged at this time labels Jan 8, 2020
@Theodus
Copy link
Contributor

Theodus commented Jan 8, 2020

@SeanTAllen
Copy link
Member Author

@Theodus I'm aware. Thus the "do not merge".

@SeanTAllen SeanTAllen force-pushed the symlink branch 6 times, most recently from 14a10c7 to eebe46d Compare January 8, 2020 19:34
@SeanTAllen
Copy link
Member Author

@kulibali So, if you pass "1" to CreateSymbolicLink (its a directory)... then it fails, if I change it to always pass U32(0) as the 3rd parameter it always passes. Not really sure what is up with that. You mentioned you would investigate.

Should I hard-code U32(0) for now and you can look at your leisure and this can be merged or do you want to look into it more first?

@SeanTAllen SeanTAllen requested a review from chalcolith January 8, 2020 20:43
@chalcolith
Copy link
Member

So theres problems with symlinks on Windows: you can only create them if you are running with elevated privileges (you have run a command window as Administrator, or spawned a process with elevated privileges, in which case a UI dialog will pop up, meaning we can't use this in CI or are using "Developer Mode" os-wide).

https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

I see a couple of problems -- if we include a symlink test on Windows then it will only work with elevated privileges, which are a problem for CI. Also, ponyup's symlinking stuff won't work without elevated privileges.

Just for illustrative purposes, the following diff makes the FilePath.symlink() method try its best to create the symlink, and adds a more extensive test.

 diff --git a/packages/files/_test.pony b/packages/files/_test.pony
 index 981668f3..c41a6f5c 100644
 --- a/packages/files/_test.pony
 +++ b/packages/files/_test.pony
 @@ -51,6 +51,7 @@ actor Main is TestList
      test(_TestFileLinesSingleLine)
      test(_TestFileLinesMultiLine)
      test(_TestFileLinesMovingCursor)
 +    test(_TestFilePathSymlink)
 
 primitive _FileHelper
 fun make_files(h: TestHelper, files: Array[String]): FilePath ? =>
 @@ -1068,3 +1069,49 @@ class _TestFileLinesMovingCursor is UnitTest
      h.assert_eq[String](fl3.next()?, "d")
      h.assert_eq[USize](file.position(), 7)
      end
 +
 +class _TestFilePathSymlink is UnitTest
 +  var tmp_dir: (FilePath | None) = None
 +
 +  fun ref set_up(h: TestHelper) ? =>
 +    tmp_dir = FilePath.mkdtemp(h.env.root as AmbientAuth, "symlink")?
 +
 +  fun ref tear_down(h: TestHelper) =>
 +    try
 +      (tmp_dir as FilePath).remove()
 +    end
 +
 +  fun name(): String => "files/FilePath.symlink"
 +
 +  fun ref apply(h: TestHelper) ? =>
 +    test_file(h)?
 +    test_directory(h)?
 +
 +  fun ref test_file(h: TestHelper) ? =>
 +    let target_path = (tmp_dir as FilePath).join("target_file")?
 +    let content = "abcdef"
 +    with target_file = CreateFile(target_path) as File do
 +      h.assert_true(
 +        target_file.write(content),
 +        "could not write to file: " + target_path.path)
 +    end
 +
 +    let link_path = (tmp_dir as FilePath).join("symlink_file")?
 +    h.assert_true(
 +      target_path.symlink(link_path),
 +      "could not create symbolic link to: " + target_path.path)
 +
 +    with link_file = OpenFile(link_path) as File do
 +      let fl = FileLines(link_file)
 +      h.assert_true(fl.has_next())
 +      h.assert_eq[String](fl.next()?, content)
 +    end
 +
 +  fun ref test_directory(h: TestHelper) ? =>
 +    let target_path = (tmp_dir as FilePath).join("target_dir")?
 +    h.assert_true(target_path.mkdir(true))
 +
 +    let link_path = (tmp_dir as FilePath).join("symlink_dir")?
 +    h.assert_true(
 +      target_path.symlink(link_path),
 +      "could not create symbolic link to: " + target_path.path)
 diff --git a/packages/files/file_path.pony b/packages/files/file_path.pony
 index 8bd61c80..6e724cca 100644
 --- a/packages/files/file_path.pony
 +++ b/packages/files/file_path.pony
 @@ -246,7 +246,7 @@ class val FilePath
 
      0 == @rename[I32](path.cstring(), new_path.path.cstring())
 
 -  fun symlink(link_name: FilePath): Bool =>
 +  fun val symlink(link_name: FilePath): Bool =>
      """
      Create a symlink to a file or directory.
      """
 @@ -255,7 +255,67 @@ class val FilePath
      end
 
      ifdef windows then
 -      0 != @CreateSymbolicLink[U8](link_name.path.cstring(), path.cstring())
 +      try
 +        var err: U32 = 0
 +
 +        // look up the ID of the SE_CREATE_SYMBOLIC_LINK privilege
 +        let priv_name = "SeCreateSymbolicLinkPrivilege"
 +        var luid: U64 = 0
 +        var ret = @LookupPrivilegeValueA[U8](
 +          USize(0),
 +          priv_name.cstring(),
 +          addressof luid)
 +        if ret == 0 then
 +          return false
 +        end
 +
 +        // get current process pseudo-handle
 +        let handle = @GetCurrentProcess[USize]()
 +        if handle == 0 then
 +          return false
 +        end
 +
 +        // get security token
 +        var token: USize = 0
 +        let rights: U32 = 0x0020 // TOKEN_ADJUST_PRIVILEGES
 +        ret = @OpenProcessToken[U8](handle, rights, addressof token)
 +        if ret == 0 then
 +          return false
 +        end
 +
 +        // try to turn on SE_CREATE_SYMBOLIC_LINK privilege
 +        // this won't work unless we are administrator or have Developer Mode on
 +        // https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/
 +        var privileges: _TokenPrivileges ref = _TokenPrivileges
 +        privileges.privilege_count = 1
 +        privileges.privileges_0.luid = luid
 +        privileges.privileges_0.attributes = 0x00000002 // SE_PRIVILEGE_ENABLED
 +        ret = @AdjustTokenPrivileges[U8](
 +          token,
 +          U8(0),
 +          privileges,
 +          U32(0),
 +          USize(0),
 +          USize(0))
 +        if ret == 0 then
 +          @CloseHandle[U8](token)
 +          return false
 +        end
 +
 +        ret = @CloseHandle[U8](token)
 +        if ret == 0 then
 +          return false
 +        end
 +
 +        let flags: U32 = if FileInfo(this)?.directory then 3 else 0 end
 +        ret = @CreateSymbolicLinkA[U8](
 +          link_name.path.cstring(),
 +          path.cstring(),
 +          flags)
 +        ret != 0
 +      else
 +        false
 +      end
      else
      0 == @symlink[I32](path.cstring(), link_name.path.cstring())
      end
 @@ -314,3 +374,11 @@ class val FilePath
 
      0 == @utimes[I32](path.cstring(), addressof tv)
      end
 +
 +struct ref _TokenPrivileges
 +  var privilege_count: U32 = 1
 +  embed privileges_0: _LuidAndAttributes = _LuidAndAttributes
 +
 +struct ref _LuidAndAttributes
 +  var luid: U64 = 0
 +  var attributes: U32 = 0

@SeanTAllen
Copy link
Member Author

@kulibali do you want to update that branch with the patch and we can not have the test on for Windows but merge it so it will at least work?

I'd say we also update the docs to note the requirement re: administrative privileges.

@chalcolith
Copy link
Member

OK, will do.

Also updates the symlink function so that it links correctly on Windows, and tries its bestest to obtain the capability to create symlinks.

Updates the documentation for `FilePath.symlink()` to note that on Windows creating symlinks requires elevated permissions.
else
0
end
0 != @CreateSymbolicLinkA[U8](link_name.path.cstring(), path.cstring(), U32(0))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the last parameter be dword?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah sorry that was the hardcoding that i left in from a test.

@chalcolith
Copy link
Member

@SeanTAllen this looks good to me

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Jan 21, 2020
@SeanTAllen SeanTAllen merged commit 470ce86 into master Jan 21, 2020
@SeanTAllen SeanTAllen deleted the symlink branch January 21, 2020 19:08
github-actions bot pushed a commit that referenced this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants