Fix FileManager's extended attribute symlink handling

[AZero13]

The if statement appears inverted: setxattr follows simlinks: lsetxattr does not.

Additionally, _extendedAttributes was ignoring simlinks entirely.

Both of these issues have been addressed.

[AZero13]

@jmschonfeld ping?

[AZero13]

@jmschonfeld @compnerd ping?

[jmschonfeld]

@jmschonfeld @compnerd ping?

It's on my list to circle back and review when I have a moment - no need to ping 🙂. Will be working my way through the handful of PRs open to review

[jmschonfeld]

@swift-ci please test

[compnerd]

The code change looks fine, needs fixes for the tests.

[AZero13]

I fixed it, but also noticed we weren't passing followSymlinks to _extendedAttributes when the test failed on Darwin so. @compnerd

[AZero13]

@jmschonfeld Can we please test and merge?

[jmschonfeld]

It looks like this was changed in a recent commit (hard to determine with force pushes rather than follow-on commits which are easier to review). Is there a reason you're switching to use the passed in size here rather than using the size of the allocation returned by the allocate function?

[AZero13]

Yes — the size variable gets updated by the second listxattr/extattr_list_* call to reflect the actual number of bytes written.

[jmschonfeld]

In that case if size is potentially different (if the file system changes and the number of attributes are removed) can we add an assertion that size <= keyList.count? In most cases I assume this shouldn't be an issue, but I'm wary that trusting this value could in some edge case inadvertently introduce a buffer overread here if size somehow becomes something larger than our allocation after it is overwritten.

[AZero13]

If anything I'd say the opposite is more likely to happen: if keyList.count changes between allocation and read then this can happen.

[jmschonfeld]

I don't think I'm fully following - in what situation would keyList.count change?

[AZero13]

Maybe this is best for another PR. I'll revert it.

[jmschonfeld]

Is there a reason why ENOTSUP is guarded for Darwin only here?

[AZero13]

Because ENOTSUP is not an error code on Linux.

I tried and it didn't compile.

[jmschonfeld]

ENOTSUP is an error code on Linux as far as I can tell if I'm reading the man pages correctly, but it looks like it has the same value as EOPNOTSUPP so there's no distinction. It looks like we also don't have a POSIXErrorCode.EOPNOTSUP for it which is likely the compilation error you were seeing (either intentionally because it's not necessary or just because nobody has ended up needing it). If that's the case, can you leave a comment about why this distinction is here so it's clear that ENOTSUP is not distinct from EOPNOTSUPP on non-Darwin platforms?

[AZero13]

Done

[AZero13]

I think this is as heavy as change now as it should get. We should merge this

I'll save the count and size replacement for another PR. That has been reverted back @jmschonfeld

[jmschonfeld]

@swift-ci test

[AZero13]

Test failed because of a permission error on linux. @jmschonfeld what to do? How am I supposed to fix that in the test

[AZero13]

Ugh I had to fix the tests to use rawvalue. The previous version passed locally, but not on the CI. My bad...

[jmschonfeld]

On the EPERM issue - yes this seems like it somewhat defeats the purpose of the test, since if permission is always denied then we're always skipping this test effectively. Have you investigated why the EPERM error is occurring or do we know why we're getting this error? Perhaps there's something about the test file that should be updated to allow this operation, or does this signify that this operation in general isn't the right direction?

[AZero13]

Yeah, I think we should just use the raw values. That is what I did.

[AZero13]

How about having it just for the probe though?

[jmschonfeld]

I'm referring to the need to check for EPERM at all here. Do you know why we're failing with an EPERM error when trying to set these attributes? I'd like to better understand in which situations this is actually possible and which situations it is expected to fail for one reason or another. We need to be careful here to make sure that introducing this change does not cause compatibility issues with clients expecting these calls to succeed if the calls begin to fail because the operation is no longer possible.

[AZero13]

The operation itself is not reliably possible across platforms/filesystems.

[AZero13]

Is the CI running in a sandbox?

[jmschonfeld]

they already can fail with EPERM depending on filesystem/platform because the implementation calls lsetxattr() for symlinks.

Right what I'm driving towards is figuring out why the can fail with that message. Is it similar to the EOPNOTSUPP case where the file system or operating system does not permit the operation on a symlink, and if so which are those? We've already excluded some operating systems for this reason, are there more? Is it a misconfiguration in the FileManagerPlayground setup that creates the files incorrectly? The problem with ignoring EPERM without understanding why is that we could be simply avoiding testing this behavior across all platforms leaving this change untested and I want to confirm where we expect this to succeed and ensure that tests are running there.

Is the CI running in a sandbox?

I'm not certain - it's possible that it is, or possible that it varies based on platform. That being said, the process running the tests creates the test files themselves and I don't think a sandbox would permit us to create files that we later would be unable to access but I have not validated that.

[AZero13]

We still check that the target is untouched, which is the core behavioral guarantee. We are not disabling the test across all platforms.

[jmschonfeld]

That's not the behavior of the code as-is today. I think the checks for the various error codes are complicating the test here a bit. I discussed with @kperryua and @jrflat and after some research and experimentation we determined that:

• The EOPNOTSUPP shouldn't be relevant here as it's only relevant for usage with sockets
• The ENOTSUP shouldn't happen here, since this test is only running on platforms where this operation should be supported
• The EPERM error seems a bit weird at first, but it seems to be an artifact of the constraint that Linux does not support user-defined xattrs on symlinks themselves

Based on the above, I think we should update the unit test to:

• No longer account for EOPNOTSUPP and ENOTSUP as they are not relevant here
• Test that we fail due to the EPERM error only on Linux
• Still validate the reading side even if the setting side fails

I took a moment to write up my suggestion for how to write this unit test based on the above plus cleaning it up just a little bit to simplify the branches and rename it to match what we're testing:

    // Extended attributes are not supported on all platforms
    #if !os(Windows) && !os(WASI) && !os(OpenBSD) && !canImport(Android)
    @Test func extendedAttributesOnSymlinks() async throws {
        let xattrKey = FileAttributeKey("NSFileExtendedAttributes")
        #if os(Linux)
        // Linux requires the user.* namespace prefix for regular files
        let attrName = "user.swift.foundation.symlinktest"
        #else
        let attrName = "org.swift.foundation.symlinktest"
        #endif
        let attrValue = Data([0xAA, 0xBB, 0xCC])
        let attrValue2 = Data([0xDD, 0xEE, 0xFF])

        try await FilePlayground {
            File("target", contents: Data("payload".utf8))
            SymbolicLink("link", destination: "target")
        }.test { fileManager in
            // First, validate that reading the attribute from the link does not read the value from the target
            do {
                try fileManager.setAttributes([xattrKey: [attrName: attrValue]], ofItemAtPath: "target")
                let targetAttrs = try fileManager.attributesOfItem(atPath: "target")
                let targetXAttrs = targetAttrs[xattrKey] as? [String: Data]
                #expect(targetXAttrs?[attrName] == attrValue)
                
                let linkAttrs = try fileManager.attributesOfItem(atPath: "link")
                let linkXAttrs = linkAttrs[xattrKey] as? [String: Data]
                #expect(linkXAttrs?[attrName] == nil)
            }

            // Attempt to set xattrs on the symlink
            #if os(Linux)
            // On Linux, user xattrs cannot be set on symlinks
            #expect(throws: CocoaError.self) {
                try fileManager.setAttributes([xattrKey: [attrName: attrValue2]], ofItemAtPath: "link")
            }
            let expectedValue: Data? = nil
            #else
            try fileManager.setAttributes([xattrKey: [attrName: attrValue2]], ofItemAtPath: "link")
            let expectedValue: Data? = attrValue2
            #endif
            
            // Ensure that reading back the xattr of the link produces the expected value
            let linkAttrs = try fileManager.attributesOfItem(atPath: "link")
            let linkXAttrs = linkAttrs[xattrKey] as? [String: Data]
            #expect(linkXAttrs?[attrName] == expectedValue)
            
            // Ensure that setting the xattr on the link did not set the xattr on the target
            let targetAttrs = try fileManager.attributesOfItem(atPath: "target")
            let targetXAttrs = targetAttrs[xattrKey] as? [String: Data]
            #expect(targetXAttrs?[attrName] == attrValue)
        }
    }
    #endif

What do you think about that? That way we always test that the reading side reads from the symlink and not the target and on the setting side it fails on Linux but otherwise succeeds and that on all platforms writing to the symlink doesn't write over the target. Does that unit test sound good to you?

[AZero13]

Yes!

[jmschonfeld]

@swift-ci please test

[AZero13]

It passed @jmschonfeld