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

.NET Core is missing suitable APIs for dealing with filesystems that contain symbolic links #26310

Open
jhudsoncedaron opened this issue Jan 13, 2018 · 22 comments
Assignees
Milestone

Comments

@jhudsoncedaron
Copy link

@jhudsoncedaron jhudsoncedaron commented Jan 13, 2018

Ref: aspnet/AspNetCore#2774 for discovery point

.NET Core is missing suitable APIs for dealing with filesystems that contain symbolic links

FileInfo does not resolve symbolic links, leading to subtle bugs. There's no API that gives information about the link target. There's other problems with the existing APIs causing them to have trouble when you call Directory.EnumerateFileSystemEntries involving having to do not one but two stat calls for each node. Might as well resolve them all at once.

Proposed API surface:

// These are deliberately set to the *nix constant values where possible.
public enum FileTypes {
      Missing = 0,
      Fifo = 0010000,
      CharacterDevice = 0020000,
      Directory = 0040000,
      BlockDevice = 0060000,
      File = 010000,
      SymbolicLink = 0120000,
      Socket = 0140000,
      SymbolicLinkMissingTarget = 0200000,
      SymbolicLinkLoop = 0400000,
      ReparsePoint = 01000000, // ReparsePoint attribute set but not a symbolic link
}

public struct FileNode {
    public string Path { get; private set; }
    public string FileName { get => System.IO.Path.GetFileName(Path); }
    public DateTime LastAccessTime { get; private set; }
    public DateTime LastAccessTimeUTC { get; private set; }
    public DateTime LastWriteTime { get; private set; }
    public DateTime LastWriteTimeUTC { get; private set; }
    public DateTime LastChangeTime { get; private set; }
    public DateTime LastChangeTimeUTC { get; private set; }
    public FileAttributes Attributes { get; private set; }
    public FileTypes FileNodeType { get; private set; }
    public string SymbolicLinkTargetPath { get; private set; }
    public bool Exists { get => FileNodeType != 0 }
    FileNode(string path, bool resolvesymboliclink)
    {
            /* The general idea of this API is it doesn't throw; just gives the appropriate information You could probably put Cer.Success on it. */
            Path = path;
            bool statpermissiondenied;
            if (resolvesymboliclink)
            {
                /* This code path would call CreateFile with only FILE_READ_ATTRIBUTES and then call GetFileInformationByHandle; on AccessDenied or PermissionDenied fall through below */
                /* on unix this would be a stat() call */
            }
            /* This code path would call FindFirstFileEx to get the file information by name from the node attribute */
            if (resolvesymboliclink && FileNodeType == FileTypes.SymbolicLink)
               FileNodeType = 0;
    }
    /* Deserialization constructor */
    FileNode(string path, FileTypes fileNodeType, FileAttributes Attributes, DateTime lastAccessTimeUTC, DateTime lastChangeTimeUTC, DateTime lastWriteTimeUTC, string symbolicLinkTargetPath);
}

public partial class File {
     public static void CreateSymbolicLink(string path, string targetPath, bool targetisdirectory = false);
}


public partial class Directory {
     // This one exists only code readability
     // The idea is the programmer would normally only pass the third parameter if it was indirection from another layer of indirection, and otherwise would call File.CreateSymbolicLink to create a symbolic link to a file and Directory.CreateSymbolicLink to create a symbolic link to a directory
     public static void CreateSymbolicLink(string path, string targetPath, bool targetisdirectory = true);
            => File.CreateSymbolicLink(path, targetPath, targetisdirectory);
}
@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jan 13, 2018

Thanks for this proposal. Do you want to recocile this with the proposals made in #25569 so we can have some all up proposal? It might make sense to move this there.

@carlreinke

cc @JeremyKuhne

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 13, 2018

@danmosemsft @JeremyKuhne I attempted a merge but didn't get too far. I updated my version with some pieces of his.

The main trouble is there's more possible file types than Attributes can handle. You really should be able to detect if you got a node of a strange type. In addition, it is a bad idea to assume that readlink() actually does the same thing as followlink() [the act of opening a link triggers followlink() which you can't call yourself]

I had actually hoped to be able to set obsolete on FileInfo on a per-project basis (with a code quality rule) so I could track down all places in legacy codebases that aren't prepared to handle symbolic links. But maybe it could be done on the constructor. It is vital to be able to actually declare on passing to FileInfo whether you want to follow the link or not.

@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 16, 2018

My first inclination would be to extend FileInfo, is there a reason we can't do that?

We should look at exposing the reparse point type on Windows if we're going to go this far with file types. Additionally we should look at how we might expose character file types on Windows as well.

Note that I want to get this merged with @carlreinke's proposal #25569 if at all possible.

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 16, 2018

@JeremyKuhne : I've been over that path a few more times. Reusing FileInfo requires a commitment to auditing all users of FileInfo in .NET Core immediately as well as adding the full file type enumeration. Adding the type enumeration is easy but will break the System.Runtime -> mscorlib shim due to differing ABI surface. I don't care if that shim breaks but you do.

@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 16, 2018

@jhudsoncedaron

requires a commitment to auditing all users of FileInfo in .NET Core immediately

If we don't change the default behavior, why would we have to audit?

Adding the type enumeration is easy but will break the System.Runtime -> mscorlib shim

I'm not sure I follow you. Adding new types and APIs to existing types doesn't break the shim- we have a ton of these in CoreFX already. The normal route is adding new things on CoreFX (1), porting to desktop (2), elevating to .NET Standard (3).

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 16, 2018

requires a commitment to auditing all users of FileInfo in .NET Core immediately
If we don't change the default behavior, why would we have to audit?

You have to audit 100% of callers anyway. I expect that something like 90% of them are not correct. I've already filed a bug in MVC with a root cause of incorrect use of FileInfo. The operative word is "immediately"; having a new type permits a staged audit as unaudited code can be found by noting its still using the old type.

@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 16, 2018

You have to audit 100% of callers anyway.

Why? The current behavior may not be ideal, but it has sufficed. Adding constructors that specify link following behavior is pretty easy to audit if the behavior matters to you.

Depreciating the *Info classes would be a massive change that would require some pretty serious, compelling evidence for the API review team to consider it. I'm happy to explore the reasoning/justification, but I'm not seeing it right now- can you help clarify the scenarios that would back depreciating these?

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 16, 2018

Lots and lots of code involving FileInfo reduces to this non-working code blob:

var info = new FileInfo(path);
if (info.Exists && (info.Attributes & FileAttribtes.Directory) == 0 && info.Length > 0)
    using (var f = new FileStream(path, ...))
    {
         // Do something with f
    }

Raymond might be pointing out the race condition, but it's more fundamental. info.Exists can lie if you intended to do much of anything with that knowledge. It turns out almost all the time you care about the attributes of the target of the link rather than the link itself. This means everybody needs to search their codebases anyway to see what the code should do in the presence of an symbolic link.

It gets worse. If some API returns a FileInfo object, that API is most likely broken already.

So we face the horns. Either every single .NET API needs to have its behavior on encountering symbolic links set to a sane behavior immediately (and just blanket saying doesn't follow links doesn't cut it--this is effectively deprecating all the APIs that return FileInfo--but int he future you can't tell which APIs have been implicitly deprecated by this and which ones have not been) or you deprecate FileInfo so that broken APIs can be identified and avoided by their potential callers. Since I don't believe the first will happen ...

@JeremyKuhne JeremyKuhne self-assigned this Jan 17, 2018
@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 17, 2018

FileInfo, etc. are documented to work on the link itself. CreateFile doesn't open the symlink itself, it opens the target. While I agree that can be confusing, it works in most cases. Your example isn't safe even if they were aligned better. Even if you check Exists on the target there is a risk the file will be gone when you try to create the FileStream around it, let alone any number of other IO related errors (access issues, volume dismounts, etc.).

We could potentially add a TargetExists or something like that to FileInfo/DirectoryInfo. (Or maybe FileInfo Target { get; } that gives you back this or the link target if applicable.)

    var info = new FileInfo(path).Target;
    if (info.Exists && (info.Attributes & FileAttribtes.Directory) == 0 && info.Length > 0)
        try
        {
          using (var f = new FileStream(path, ...))
          {
             // Do something with f
          }
        }
        catch (Exception e)
        {
          // handle IO errors
        }

I absolutely agree that we need better symbolic link support, but I don't feel depreciating existing APIs is the way to do it.

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 17, 2018

I constructed that example because that's the kind of nonsense I keep on finding in .NET Core itself when code falls down in the presence of symbolic links. Most callers aren't calling it because they want the link's attributes. Most callers are calling it because they want the file's attributes and there's no other API to do so. Your "it works in most cases" is because people are largely not using symbolic links yet. What's it going to take to get all of the code audited?

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 17, 2018

It occurred to me that this api var info = new FileInfo(path).Target; is really bad as it results in a call to lstat() followed by stat() rather than just a call to stat() (on Windows a call to FindFirstFile() followed by GetFileInformationByHandle() rather than just GetFileInformationByHandle()).

@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 17, 2018

Most callers aren't calling it because they want the link's attributes.

This is documented and follows the behavior of the underlying Win32 APIs. I personally would have probably designed things differently, but we now have decades of code written with this behavior. Breaking with that and depreciating a large swath of APIs because it is considered confusing isn't the right answer, imho. Making it more discoverable is.

there's no other API to do so.

There will be- I'm fully committed to getting symbolic link handling into the framework.

What's it going to take to get all of the code audited?

Open issues in the case of .NET code. Writing code that doesn't correctly follow the documentation or allow for the completely transient nature of IO happens, and would be a bug with or without symbolic links involved.

It occurred to me that this api var info = new FileInfo(path).Target; is really bad

How is it so bad? You still have to make multiple calls to go from path to finalPath. Can you give more detailed code samples?

@JeremyKuhne

This comment has been minimized.

Copy link
Member

@JeremyKuhne JeremyKuhne commented Jan 17, 2018

Note that there is also a need to identify exactly what type of reparse point is in play for Windows. Understanding whether the reparse point is actually a symbolic link is a critical first step, but digging deeper into reparse point types is needed. A specific scenario is that Windows 10 has a new type of reparse point for OneDrive Files On-Demand.

I'll open up an issue for how to expose platform specific data so we can discuss general approach. I'll link to this from there.

@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 17, 2018

You still have to make multiple calls to go from path to finalPath. Can you give more detailed code samples?

No, you don't. In fact, trying to resolve final path yourself is a bug. readlink() might not resolve the same file as open().

You go directly to final path by making the appropriate API calls. Windows:

try {
} finally {
    /* this code must not be split by async throw */
    IntPtr MinusOne = IntPtr.Zero - 1;
    IntPtr h = NativeMethods.CreateFile(pathname, 128 /* FILE_READ_ATTRIBUTES */, 7 /* FILE_SHAARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE */, IntPtr.Zero, 3 /* OPEN_EXISTING */, 0x00100000 /* FILE_FLAG_NO_RECALL */, MinusOne);
    if (h != MinusOne) {
        struct NativeMethods.BY_HANDLE_FILEINFORMATION nfi;
        bool r = NativeMethods.GetFileInformationByHandle(h, &nfi);
        NativeMethods.CloseHandle(h);
        if (!r) throw new IOException(new System.ComponentModel.Win32Exception); /* should not happen */
        /* Populate FileInfo from nfi */
    } else {
        /* File not found or no permission to resolve link -- check GetLastError() to find out which */
    }
}

Linux & Mac OS (they would differ in the decl of the stat structure)

struct NativeMethods.stat statbuf;
if (stat(pathname, &statbuf)) {
    /* file not found or link can't be followed -- check errno */
} else {
    /* Populate FileInfo from statbuf */
}
@jhudsoncedaron

This comment has been minimized.

Copy link
Author

@jhudsoncedaron jhudsoncedaron commented Jan 17, 2018

I spent a little time looking for cases. I found a lot of cases where I could not track because the code spanned repos. I only found one case that appeared to work, and that by chance. It blows up on encountering a symlink to a non-extant directory when given a path within it but maybe that's intended.

Tries to set ACls on a symbolic link. Unix symbolic links don't have ACLs. Trying to do so would set the ACL on the target instead.

corefx/src/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.cs

Definitely does the wrong thing on encountering symbolic links; can be thrown into a stack overflow.

corefx/src/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.cs

    private static void DoCreateFromDirectory(string sourceDirectoryName, string destinationArchiveFileName,
                                              CompressionLevel? compressionLevel, bool includeBaseDirectory,

Encoding entryNameEncoding)

Code only makes sense if FileInfo were resolving link target paths; otherwith the Path functions do its job

coreclr/src/ToolBox/SOS/DacTableGen/main.cs

Certainly fails in the presence of symbolic links but hard to see what it should do

Razor/src/Microsoft.AspNetCore.Razor.Language/FileSystemRazorProject.cs

Tries to listen to symbolic links for changes. Hard to say if the code works or not.

corefx/src/System.Runtime.Caching/src/System/Runtime/Caching/FileChangeNotificationSystem.cs

@iSazonov

This comment has been minimized.

Copy link
Contributor

@iSazonov iSazonov commented Aug 29, 2018

In PowerShell we already dynamically add a Target property to FileInfo/DirectoryInfo objects.

@xp-1000

This comment has been minimized.

Copy link

@xp-1000 xp-1000 commented Dec 12, 2018

Hello,
Is there any news on this ? at least a workaround to allow netcore application to follow symlinks ?
It seems to be a basic feature on linux environment.
Thanks

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Dec 12, 2018

@xp-1000 nope, but perhaps we can pick this up in January. We do want this, and if we can get to an approved API shape, implementation can follow.

@xp-1000

This comment has been minimized.

Copy link

@xp-1000 xp-1000 commented Dec 12, 2018

understood. thanks for the quick answer. I will continue to follow this issue.

@joshudson

This comment has been minimized.

Copy link
Contributor

@joshudson joshudson commented Dec 13, 2018

I am more than half way through a real implementation of this suitable as an independent nuget package but I am stopped due to not being able to run personal Windows machines very well. I have specific ADA problems that makes it hard without on-site support.

@joshudson

This comment has been minimized.

Copy link
Contributor

@joshudson joshudson commented Jan 1, 2019

@xp-1000 : Here's something to play with

https://github.com/joshudson/Emet/tree/master/FileSystems

I published Emet.FileSystems 0.0.1-alpha1 on nuget; it's still verifying right now.

@iSazonov iSazonov referenced this issue May 2, 2019
7 of 10 tasks complete
@stephentoub stephentoub modified the milestones: Future, 5.0 Jul 18, 2019
@lastlink

This comment has been minimized.

Copy link

@lastlink lastlink commented Jul 20, 2019

@joshudson could you add some documentation to you library, is it possible to edit file attributes such as author.

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this issue Oct 24, 2019
Today `async ValueTask/ValueTask<T>` methods use builders that special-case the synchronously completing case (to just return a `default(ValueTask)` or `new ValueTask<T>(result))` but defer to the equivalent of `async Task/Task<T>` for when the operation completes asynchronously. This, however, doesn't take advantage of the fact that value tasks can wrap arbitrary `IValueTaskSource/IValueTaskSource<T>` implementations.

This commit gives `AsyncValueTaskMethodBuilder` and `AsyncValueTaskMethodBuilder<T>` the ability to use pooled `IValueTaskSource/IValueTaskSource<T>` instances, such that calls to an `async ValueTask/ValueTask<T>` method incur 0 allocations (ammortized) as long as there's a cached object available.

Currently the pooling is behind a feature flag, requiring opt-in via the DOTNET_SYSTEM_THREADING_POOLASYNCVALUETASKS environment variable (setting it to "true" or "1").  This is done for a few reasons:
- There's a breaking change here, in that while we say/document that `ValueTask/ValueTask<T>`s are more limited in what they can be used for, nothing in the implementation actually stops a `ValueTask` that was wrapping a `Task` from being used as permissively as `Task`, e.g. if you were to `await` such a `ValueTask` multiple times, it would happen to work, even though we say "never do that". This change means that anyone who was relying on such undocumented behaviors will now be broken. I think this is a reasonable thing to do in a major release, but I also want feedback and a lot of runway on it.
- Policy around pooling. Pooling is always a difficult thing to tune. Right now I've chosen a policy that limits the number of pooled objects per state machine type to an arbitrary multiple of the processor count, and that tries to strike a balance between contention and garbage by using a spin lock and if there's any contention while trying to get or return a pooled object, the cache is ignored. We will need to think hard about what policy to use here. It's also possible it could be tuned per state machine, e.g. by having an attribute that's looked up via reflection when creating the cache for a state machine, but that'll add a lot of first-access overhead to any `async ValueTask/ValueTask<T>` method.  For now, it's tunable via the `DOTNET_SYSTEM_THREADING_POOLASYNCVALUETASKSLIMIT` environment variable, which may be set to the maximum number of pooled objects desired per state machine.
- Performance validation.  Preliminary numbers show that this accomplishes its goal, having on-par throughput with the current implementation but with significantly less allocation.  That needs to be validated at scale and across a variety of workloads.
- Diagnostics.  There are several diagnostics-related abilities available for `async Task/Task<T>` methods that are not included here when using `async ValueTask/ValueTask<T>` when pooling.  We need to evaluate these (e.g. tracing) and determine which pieces need to be enabled and which we're fine omitting.

Before shipping .NET 5, we could choose to flip the polarity of the switch (making it opt-out rather than opt-in), remove the fallback altogether and just have it be always on, or revert this change, all based on experimentation and data we receive between now and then.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub added a commit that referenced this issue Oct 24, 2019
Today `async ValueTask/ValueTask<T>` methods use builders that special-case the synchronously completing case (to just return a `default(ValueTask)` or `new ValueTask<T>(result))` but defer to the equivalent of `async Task/Task<T>` for when the operation completes asynchronously. This, however, doesn't take advantage of the fact that value tasks can wrap arbitrary `IValueTaskSource/IValueTaskSource<T>` implementations.

This commit gives `AsyncValueTaskMethodBuilder` and `AsyncValueTaskMethodBuilder<T>` the ability to use pooled `IValueTaskSource/IValueTaskSource<T>` instances, such that calls to an `async ValueTask/ValueTask<T>` method incur 0 allocations (ammortized) as long as there's a cached object available.

Currently the pooling is behind a feature flag, requiring opt-in via the DOTNET_SYSTEM_THREADING_POOLASYNCVALUETASKS environment variable (setting it to "true" or "1").  This is done for a few reasons:
- There's a breaking change here, in that while we say/document that `ValueTask/ValueTask<T>`s are more limited in what they can be used for, nothing in the implementation actually stops a `ValueTask` that was wrapping a `Task` from being used as permissively as `Task`, e.g. if you were to `await` such a `ValueTask` multiple times, it would happen to work, even though we say "never do that". This change means that anyone who was relying on such undocumented behaviors will now be broken. I think this is a reasonable thing to do in a major release, but I also want feedback and a lot of runway on it.
- Policy around pooling. Pooling is always a difficult thing to tune. Right now I've chosen a policy that limits the number of pooled objects per state machine type to an arbitrary multiple of the processor count, and that tries to strike a balance between contention and garbage by using a spin lock and if there's any contention while trying to get or return a pooled object, the cache is ignored. We will need to think hard about what policy to use here. It's also possible it could be tuned per state machine, e.g. by having an attribute that's looked up via reflection when creating the cache for a state machine, but that'll add a lot of first-access overhead to any `async ValueTask/ValueTask<T>` method.  For now, it's tunable via the `DOTNET_SYSTEM_THREADING_POOLASYNCVALUETASKSLIMIT` environment variable, which may be set to the maximum number of pooled objects desired per state machine.
- Performance validation.  Preliminary numbers show that this accomplishes its goal, having on-par throughput with the current implementation but with significantly less allocation.  That needs to be validated at scale and across a variety of workloads.
- Diagnostics.  There are several diagnostics-related abilities available for `async Task/Task<T>` methods that are not included here when using `async ValueTask/ValueTask<T>` when pooling.  We need to evaluate these (e.g. tracing) and determine which pieces need to be enabled and which we're fine omitting.

Before shipping .NET 5, we could choose to flip the polarity of the switch (making it opt-out rather than opt-in), remove the fallback altogether and just have it be always on, or revert this change, all based on experimentation and data we receive between now and then.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.