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

proposal: path/filepath: add IsReserved to identify Windows reserved filenames #57152

Closed
neild opened this issue Dec 7, 2022 · 24 comments
Closed

Comments

@neild
Copy link
Contributor

neild commented Dec 7, 2022

On Windows, a number of filenames are reserved for use by the system, such as "CON", "COM1", and "CONIN$". The set of reserved names is subtle and varies by Windows version; some versions reserve names with extensions such as "con.txt" while others do not.

The new filepath.IsLocal function (#56219) returns false when provided a reserved filename on Windows. However, we don't have a function which only identifies reserved filenames. This seems like a useful thing to have. We already have one Windows-specific function in filepath (filepath.VolumeName), so adding another may be reasonable.

The proposal:

// IsReserved reports whether the path is a reserved name.
//
// On Windows, it returns true if any element of path contains a reserved name such as "NUL".
// On other platforms, it always returns false.
func IsReserved(path string) bool
@neild neild added the Proposal label Dec 7, 2022
@gopherbot gopherbot added this to the Proposal milestone Dec 7, 2022
@ianlancetaylor
Copy link
Contributor

Who do we expect to call IsReserved? It seems like the only people who would call it are those specifically programming for Windows. Therefore, would this be more appropriate in the x/sys/windows package?

@ianlancetaylor
Copy link
Contributor

CC @golang/windows

@bcmills
Copy link
Contributor

bcmills commented Dec 8, 2022

would this be more appropriate in the x/sys/windows package?

For portable programs, it seems most useful to have IsReserved always be defined and always return false outside of Windows. Otherwise, the program would need otherwise-unnecessary build constraints just to implement its own copy of IsReserved exactly as proposed.

I don't think x/sys/windows is defined for non-Windows platforms today. It might make sense to define the non-Windows-specific parts of that package (such as StringToUTF16) on other platforms too, but I don't think this proposal is really the place to consider that much broader change.

So I think path/filepath really is the right place for this function.

@neild
Copy link
Contributor Author

neild commented Dec 8, 2022

As @bcmills says, for people writing portable programs it seems more useful to have an IsReserved that always returns false on platforms with no reserved names.

Another motivating case is #57151 (filepath.FromFS), which is another function which needs to identify reserved names. If we add it to the standard library, then we'll have two functions that have special handling of reserved names (FromFS and IsLocal) but none that specifically identifies them. If we don't add FromFS to the standard library, then it will be difficult to implement outside it without a good way to identify reserved names.

@iwdgo
Copy link
Contributor

iwdgo commented Jan 7, 2023

Function was re-written recently in package path/filepath for windows.

// isReservedName reports if name is a Windows reserved device name or a console handle.
// It does not detect names with an extension, which are also reserved on some Windows versions.
//
// For details, search for PRN in
// https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file.
func isReservedName(name string) bool {

I do not know if proposal is complete.

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

With IsLocal having been added, do we still need IsReserved?
Would it have an implementation on other systems, like rejecting names with NUL bytes on Linux?

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@neild
Copy link
Contributor Author

neild commented Feb 1, 2023

I believe IsReserved would only have an implementation on Windows, like filepath.VolumeName.

A name with NUL bytes is invalid, not reserved.

I could see an argument for calling "." and ".." reserved.

A standard library function should be some combination of broadly useful or difficult to implement. A function which is trivial to implement may be worth including if it is very widely used, and a function which is seldom used may be worth including if those rare uses would be painful without it.

I believe IsReserved is useful only in very limited situations, but it is really quite difficult to implement and the consequences of getting it wrong can be hazardous. We've been through several iterations of our implementation of Windows reserved name detection, adding detection of reserved names with a directory component (./NUL), reserved names with an extension (but only on some versions of Windows!), and the reserved names CONIN$ and CONOUT$. Our current implementation includes a call out to syscall.FullPath to ask the operating system to answer some of the tricky edge cases.

The complexity of this function argues to me that we should make it available on its own, not just as part of a broader API like IsLocal.

Perhaps we could put this in some external package (although probably not x/sys/windows as @bcmills points out) and vendor it into the standard library instead. However, the presence of filepath.VolumeName does set (possibly unfortunate) precedent for putting Windows-specific functionality into filepath.

@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

What does IsReserved("foo/nul/bar") return?
Does IsReserved("#c") return true on Plan 9?
On Plan 9 "./#c" is not reserved even though "#c" is.

Does IsReserved mean "this path doesn't mean what it looks like?"

@neild
Copy link
Contributor Author

neild commented Feb 22, 2023

IsReserved("foo/nul/bar") returns true, I think, but I'd entertain arguments for false.

I don't know about Plan 9. Do you have a reference to how #-prefixed paths work? I haven't been able to find one. Can I ReadDir("#") to list all the paths with a #-prefix?

On Windows, IsReserved would mean a name reserved by Windows as documented in https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file, or a name which has special meaning to CreateFile as documented in https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#consoles.

"Doesn't mean what it looks like" is a good characterization. Or "not actually part of the filesystem".

@ianlancetaylor
Copy link
Contributor

For Plan 9, search for #c in https://www.usenix.org/legacy/publications/compsystems/1995/sum_pike.pdf . I don't think that ReadDir("#") works.

@neild
Copy link
Contributor Author

neild commented Feb 23, 2023

The root directory of the tree served by a device driver can be accessed by the notation #c, where c is a unique character (typically a letter) identifying the type of the device.

I could see an argument for calling #c a volume name (which would have the effect of making filepath.Clean("#c/../x") = "#c/x"), but I don't think it's a reserved name.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

Yes, #c on Plan 9 is a lot like a volume name on Windows.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

What about IsReserved("..")? .. is magical in a similar way to NUL in that it is not an ordinary name and does not have an ordinary meaning. It would fall into 'doesn't mean what it looks like' unless the looker knows about dotdot (which maybe everyone does).

Can we write a precise definition of what "Reserved" means?

@rsc
Copy link
Contributor

rsc commented Mar 8, 2023

Now that I think more about the volume name question, does IsReserved("C:") return true? Does IsReserved("C:/foo") return true? If not, how does code that checks paths with IsReserved avoid being tricked into writing to absolute paths starting with volume names? filepath.IsLocal already does that I guess. But then I'm confused about what the use case is. I should use IsLocal instead of IsReserved almost all the time.

However, we don't have a function which only identifies reserved filenames. This seems like a useful thing to have.

Is there a clearer use case?

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

Same question as last week: do we really need this function to be exported at all?

@neild
Copy link
Contributor Author

neild commented Mar 15, 2023

A good definition might be: A reserved file name is one which cannot be used to create a file with that name.

For example, on Windows it is not possible to create a file named NUL, COM1, or CONIN$. It is possible to open these names and retrieve a file handle, but the names are reserved for use by the operating system.

Under this definition, I think the names . and .. are reserved, since you can't create a directory or file named . or ...

Volume names are not reserved, because a volume name isn't a file name. IsReserved("c:") is false because it doesn't name a file. IsReserved("C:/foo") is also false, because you can create a file named "C:/foo".

I'm not certain that we need this function. My main argument in favor of it is still the one from #57152 (comment): Very few users need this, but it's incredibly hard to get right and risky to get wrong.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

What about IsReserved("/")? I have a hard time explaining why the answers that IsReserved returns for "/", ".", "..", "c:\", and "c:" would not all be the same, whatever it ends up being.

If users already have IsLocal (which already rejects all these things like NUL and COMIN$), when do users need IsReserved instead of IsLocal?

@neild
Copy link
Contributor Author

neild commented Mar 29, 2023

Consider a user who wants to implement #57151 (a version of filepath.FromSlash that validates its input). This function is willing to accept absolute paths and paths which escape the current directory (so /a and ../a are fine), but wants to disallow paths that contain a volume name or reference a device file.

You can do this with IsLocal, but it's not trivial since you need to convert absolute paths and ones that escape the current directory into a local form.

I don't think it's useful to call / reserved; the root directory isn't reserved, it's just the root directory. It behaves like any other directory, except that you can't delete it.

The motivation for IsReserved is that while everyone knows what /, ., and .. mean (for some definition of "everyone"), Windows has a bunch of reserved filenames that behave in strange ways and are not easy to discover.
Perhaps the best way to specify IsReserved is to just list exactly what it does:

// IsReserved reports whether the path is reserved.
//
// On Windows, a path is reserved if it contains a path component with the name CONIN$, CONOUT$,
// CON, PRN, AUX, NUL, COM0, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9,
// LPT0, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, or LPT9.
//
// On some versions of Windows, names with an extension such as NUL.txt are also reserved.
//
// On non-Windows platforms, IsReserved returns false for all paths.

@ianlancetaylor
Copy link
Contributor

I feel like we should call this IsSpecialIfRunningWindows. Or IsMagicDeviceName.

@rsc
Copy link
Contributor

rsc commented Apr 5, 2023

@neild and I discussed and suggest declining for now until we have more compelling use cases. Most programs will be well served by FromFS/Localize.

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Apr 12, 2023
@golang golang locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants