Expose file attributes of OneDrive placeholders

[sba923]

PR Summary

Fix #8315.

PowerShell Core must "expose" the OneDrive placeholders so as to make all their attributes visible

This is done by changing the "placeholder compatibility mode" to PHCM_EXPOSE_PLACEHOLDERS

PR Context

Without this change, PowerShell Core isn't backwards-compatible with Windows PowerShell: the attributes returned for OneDrive placeholders are not the same as in PS5.1

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
    • Add [feature] to your commit messages if the change is significant or affects feature tests

[vexx32]

Nice work!

[iSazonov]

I'm not sure that we can add tests.

[msftclas]

All CLA requirements met.

[sba923]

Strange: I've updated the code, and CodeFactor (still) reports issues that refer to the previous version of the code... What am I missing?

[vexx32]

Hmm... Looks like it's complaining mainly about the constants not being marked as internal/private/public. I think Ilya recommended they should all be internal?

You can probably ignore the complaints about names with underscores, though; I think that's a common style in this repo and they'll probably adjust the style rules for it sooner or later. 😄

[sba923]

I was actually fighting... my git ignorance.

Now that my changes have been pushed again, CodeFactor has run again and reports only 8 issues.

Checking them out.

[sba923]

My understanding of what I see is that CodeFactor issues are blocking, so I will have to get rid of those underscores!

[vexx32]

@sba923 most of the time, yeah, but this isn't the first time I've seen it come up with issues that the PS team and maintainers don't necessarily agree with.

If you're more or less done (check that failing test on the Windows CI and see if you can figure that one out and resolve if it's something your changes might have affected), just remove the WIP flag and ping the codeowners for further review.

If those issues are a problem they'll let you know. 🙂

[sba923]

This is getting tough. I've tried commenting out the core of the change... and Windows CI doesn't fail anymore.

This means the "placeholder mode" has an impact on the behavior of New-Item -Type Symboliclink…

I'm going back to my C# test tool for the placeholder issue and see what I can understand from there.

[sba923]

I think I'm gonna need some help...

Current placeholder compatibility mode: 1 (PHCM_DISGUISE_PLACEHOLDER)
Attributes for 'C:\Users\steph\OneDrive\test\alwaysavailable.xml': 0x00080020
Creation of symbolic link 'c:\temp\link1' to 'd:\tmp' with flags 1 FAILED
Creation of symbolic link 'c:\temp\link2' to 'd:\tmp' with flags 3 SUCCEEDED
Setting placeholder compatibility mode to: 2 (PHCM_EXPOSE_PLACEHOLDERS)
Attributes for 'C:\Users\steph\OneDrive\test\alwaysavailable.xml': 0x00080420
Creation of symbolic link 'c:\temp\link3' to 'd:\tmp' with flags 1 FAILED
Creation of symbolic link 'c:\temp\link4' to 'd:\tmp' with flags 3 SUCCEEDED

Everything's expected (my machine has developer mode enabled).

The behavior of the CreateSymlink() is unaffected by the placeholder mode.

How could I debug that failing test in CI?

[sba923]

??? reverted to the initial code, Windows CI now succeeds...

[iSazonov]

@sba923
CodeFactor is not mandatory.
Please use our code guidelines https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/coding-guidelines.md#naming-conventions
Maintainers will ask you to fix style issues if needed.

[sba923]

I've changed the code to get rid of the last two issues with underscores, but I could revert to the names from the SDK #define's to make the code more understandable. What do you think?

[iSazonov]

@sba923 Yes, our guidelines say:

The only exception is for interop code where the constant should exactly match the name and value of the code you are calling via interop (e.g. const int ERROR_SUCCESS = 0).

[sba923]

OK, will change that back (which will bring two CodeFactor issues back).

BTW you asked me earlier to change sbyte to char because https://docs.microsoft.com/en-us/cpp/dotnet/calling-native-functions-from-managed-code?view=vs-2017 says the managed type for CHAR is char. But for these particular APIs, some return values (that I don't use in the code for now) are negative, something char doesn't allow--that's why my prototype C# code was using sbyte (I was also following https://manski.net/2012/06/pinvoke-tutorial-passing-parameters-part-3/#marshalling-primitive-data-types . Should I stick to char or revert to sbyte to be "more compatible with the underlying native APIs" and more "future proof"?

[iSazonov]

@sba923 My understanding is that C++ CHAR is C# char and has 2 byte length so we can not use sbyte. I am ok with using char - we can always use cast to int to check result code.

[vexx32]

@iSazonov if the C++ CHAR has 2-byte length and as @sba923 says is returning some negative values in this instance, wouldn't a short type be preferable here?

[sba923]

IIRC CHAR is a typedef for C/C++ char which is a signed 8-bit type, so not the same as C# char which is a UTF-16 2-byte character. TBC once I'm in front of my PC 😜

[iSazonov]

I expected that there is auto marshalling for char but it is not blittable. So you are right and it should be sbyte.

[sba923]

Everything's green, except for CodeFactor.

What's still blocking the PR from merging?

[iSazonov]

@sba923 CodeFactor is not mandatory. @anmenaga was assigned so wait for his conclusion - merge or ask additional review.

[sba923]

OK, fine.

[sba923]

I've come up with the following crude test script to be used for non-CI / interactive testing:

# test fix for PowerShell Core issue #8315
# https://github.com/PowerShell/PowerShell/issues/8315
#
# fix implemented by PR #8745
# https://github.com/PowerShell/PowerShell/pull/8745
#
# Can't be tested by Windows CI since it lacks the proper environment
# (OneDrive desktop client sync directory)

$iswinps = ($null, 'Desktop') -contains $PSVersionTable.PSEdition
if ($iswinps)
{
    Write-Host("This script requires PowerShell Core")
    Exit(1)
}


if ( `
    ($null -ne $env:OneDrive) `
    -and ($env:OneDrive -ne '') `
    -and (Test-Path -LiteralPath $env:OneDrive -PathType Container) `
)
{
    # create a test file under the OneDrive sync folder
    $testfile = Join-Path -Path $env:OneDrive -ChildPath ("pwsh-8315-test-file-{0:yyyyMMddHHmmss}.txt" -f [datetime]::Now)
    if (!(Test-Path -LiteralPath $testfile))
    {
        "some data" | Out-File -LiteralPath $testfile -Encoding utf8
    }

    # make the testfile "Always available on this device"
    & attrib +P -U $testfile

    # wait until end of sync so that attributes don't change between the two times they're retrieved below
    Start-Sleep  -Seconds 15

    # get the attributes using the current (PowerShell Core) host
    $pscoreattributes = "0x{0:x8}" -f ([int](Get-Item -LiteralPath $testfile).Attributes)

    # get the attributes using Windows PowerShell
    $winpsattributes = & powershell -noprofile -command ('"{0x{0:x8}}" -f ([int] (Get-Item -LiteralPath "' + $testfile + '").Attributes)')

    # check they're identical
    if ($pscoreattributes -ne $winpsattributes)
    {
        Write-Host -ForegroundColor Red ("FAILURE: PowerShell Core attributes for '{0}' = {1}, Windows PowerShell attributes = {2}" -f $testfile, $pscoreattributes, $winpsattributes)
    }
    else
    {
        Write-Host -ForegroundColor Green ("SUCCESS: PowerShell Core attributes for '{0}' = {1}, Windows PowerShell attributes = {2}" -f $testfile, $pscoreattributes, $winpsattributes)
    }

    Remove-Item -Force -LiteralPath $testfile
}

Would there be a way to add this to the codebase?

[sba923]

Code changes made.

.\test\common\markdown\markdown-link.tests.ps1 fails in CI but succeeds locally. What's going on?

[iSazonov]

I can not open https://blogs.msdn.microsoft.com/kebab/2013/06/09/an-introduction-to-error-handling-in-powershell/ - seems the site temporary broken in the time.

[sba923]

How's that related to the failing test?

[sba923]

How's that related to the failing test?

[iSazonov]

The link is from the fail test. Please ignore this - we'll restart the test later.

[sba923]

Oops... Hadn't checked what those links were...

[sba923]

Codacy has been pending for 7 hours... anything wrong there?

What about my test script question?

[vexx32]

Looking at a few other PRs, it seems Codacy reviews might have been disabled for the time being.

Not sure about any kind of interactive test scripts, though. It might be worth looking into whether you can manually create a filesystem object with the placeholder attributes set, and then testing that the cmdlets properly retrieve that data back.

[sba923]

Codacy doesn't look disabled to me, it says "Pending — Hang in there, Codacy is reviewing your Pull request."

I will look into the testing approach you're suggesting. The code I shared earlier assumes the test file is created under the OneDrive sync folder, which causes the OneDrive plumbing to set some of the attributes that get maskes by the "placeholder compatibility" mechanism. I could indeed try to create a file "anywhere" with those bits set, and see what PSCore and WinPS return for that same file.

[vexx32]

Yeah, I'm sure if you push another commit it probably will go away. It's just an issue of Github only updating those when a commit is made, or the tool itself updates the status (which it won't if it's been disabled).

[anmenaga]

Restarted 'PowerShell-CI-static-analysis' as the error is unrelated and was fixed by another PR.

[anmenaga]

@sba923 Thank you for your contribution!

[sba923]

You're very welcome!

We haven't addressed the question of potentially keeping my (interactive) test script somewhere in the source tree....

[iSazonov]

@adityapatwardhan Could you please help with the test? Make sense to add it?

[sba923]

Note that I haven't been able to find a way to create test files with the appropriate attributes, outside of the OneDrive client's sync folder, from the test script (yet). Without this, it's not possible to run the test in an automated way.

FWIW, here's the current version of the script:

# test fix for PowerShell Core issue #8315
# https://github.com/PowerShell/PowerShell/issues/8315
#
# fix implemented by PR #8745
# https://github.com/PowerShell/PowerShell/pull/8745
#
# Can't be tested by Windows CI since it lacks the proper environment
# (OneDrive desktop client sync directory)

$iswinps = ($null, 'Desktop') -contains $PSVersionTable.PSEdition
if ($iswinps)
{
    Write-Host -ForegroundColor Yellow ("This script requires PowerShell Core")
    Exit(1)
}


if ($null -eq (Get-Process -Name 'OneDrive' -ErrorAction SilentlyContinue))
{
    Write-Host -ForegroundColor Yellow ("OneDrive not running")
    Exit(1)
}

if ( `
    ($null -ne $env:OneDrive) `
        -and ($env:OneDrive -ne '') `
        -and (Test-Path -LiteralPath $env:OneDrive -PathType Container) `
)
{
    # create a test file under the OneDrive sync folder
    $testfile = Join-Path -Path $env:OneDrive -ChildPath ("pwsh-8315-test-file-{0:yyyyMMddHHmmss}.txt" -f [datetime]::Now)
    if (!(Test-Path -LiteralPath $testfile))
    {
        "some data" | Out-File -LiteralPath $testfile -Encoding utf8
    }

    # make the testfile "Always available on this device"
    & attrib +P -U $testfile

    # wait until end of sync so that attributes don't change between the two times they're retrieved below
    Start-Sleep  -Seconds 15

    # get the attributes using the current (PowerShell Core) host
    $pscoreattributes = "0x{0:x8}" -f ([int](Get-Item -LiteralPath $testfile).Attributes)

    # get the attributes using Windows PowerShell
    $winpsattributes = & powershell -noprofile -command ('"{0x{0:x8}}" -f ([int] (Get-Item -LiteralPath "' + $testfile + '").Attributes)')

    # check that the attributes contain at least one of the bits that are masked off if the placeholder mode is set to PHCM_DISGUISE_PLACEHOLDER
    if (($winpsattributes -band ([System.IO.FileAttributes]::SparseFile -bor [System.IO.FileAttributes]::ReparsePoint)) -eq 0)
    {
        Write-Host -ForegroundColor Red ("FAILURE: Cannot create suitable test file: attributes for '{0}' = {1}", $testfile, $winpsattributes)
    }
    else
    {
        # check they're identical
        if ($pscoreattributes -ne $winpsattributes)
        {
            Write-Host -ForegroundColor Red ("FAILURE: PowerShell Core attributes for '{0}' = {1}, Windows PowerShell attributes = {2}" -f $testfile, $pscoreattributes, $winpsattributes)
        }
        else
        {
            Write-Host -ForegroundColor Green ("SUCCESS: PowerShell Core attributes for '{0}' = {1}, Windows PowerShell attributes = {2}" -f $testfile, $pscoreattributes, $winpsattributes)
        }

    }


    Remove-Item -Force -LiteralPath $testfile
}