[PATCH] Compatibility improvement to reparse point handling, v2
Corinna Vinschen
corinna-cygwin@cygwin.com
Mon Jun 12 10:27:00 GMT 2017
Hi Joe,
thanks for the patch. Review inline.
On Jun 9 15:44, Joe Lowe wrote:
> 2nd pass at reparse point handling patch.
> [...]
> static inline int
> readdir_check_reparse_point (POBJECT_ATTRIBUTES attr)
> {
> - DWORD ret = DT_UNKNOWN;
> + int ret = DT_UNKNOWN;
Given that d_type is an unsigned char type, maybe it would be better to
change the return type of the function (and `ret') accordingly instead.
> IO_STATUS_BLOCK io;
> HANDLE reph;
> UNICODE_STRING subst;
> @@ -197,20 +195,11 @@ readdir_check_reparse_point (POBJECT_ATTRIBUTES attr)
> &io, FSCTL_GET_REPARSE_POINT, NULL, 0,
> (LPVOID) rp, MAXIMUM_REPARSE_DATA_BUFFER_SIZE)))
> {
> - if (rp->ReparseTag == IO_REPARSE_TAG_MOUNT_POINT)
> - {
> - RtlInitCountedUnicodeString (&subst,
> - (WCHAR *)((char *)rp->MountPointReparseBuffer.PathBuffer
> - + rp->MountPointReparseBuffer.SubstituteNameOffset),
> - rp->MountPointReparseBuffer.SubstituteNameLength);
> - /* Only volume mountpoints are treated as directories. */
> - if (RtlEqualUnicodePathPrefix (&subst, &ro_u_volume, TRUE))
> - ret = DT_DIR;
> - else
> - ret = DT_LNK;
> - }
> - else if (rp->ReparseTag == IO_REPARSE_TAG_SYMLINK)
> - ret = DT_LNK;
> + /* Need to agree with path_conv, so use common helper logic.
> + */
> + ret = get_reparse_point_type (rp, false/*remote*/, &subst);
> + if (ret == DT_UNKNOWN)
> + ret = DT_DIR;
This looks not right to me. Every unknown type is a dir? Unknown is
unknown and should stay this way, that's why the DT_UNKNOWN has been
defined as valid type.
> @@ -2027,13 +2016,23 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
>
> InitializeObjectAttributes (&attr, fname, pc.objcaseinsensitive (),
> get_handle (), NULL);
> - de->d_type = readdir_check_reparse_point (&attr);
> - if (de->d_type == DT_DIR)
> + switch (readdir_check_reparse_point (&attr))
> {
> - /* Volume mountpoints are treated as directories. We have to fix
> - the inode number, otherwise we have the inode number of the
> - mount point, rather than the inode number of the toplevel
> - directory of the mounted drive. */
> + case DT_LNK:
> + de->d_type = DT_LNK;
> + break;
> + case DT_UNKNOWN:
> + /* Unknown reparse point type: HSM, dedup, compression, ...
> + Treat as normal directory. */
> + de->d_type = DT_DIR;
Same here.
> diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
> index 7d1d23d72..cd62355d7 100644
> --- a/winsup/cygwin/path.cc
> +++ b/winsup/cygwin/path.cc
> @@ -2261,6 +2261,86 @@ symlink_info::check_sysfile (HANDLE h)
> return res;
> }
>
> +static bool
> +check_reparse_point_target (PUNICODE_STRING subst)
> +{
> + /* Native junction reparse points, or native non-relative
> + symbolic links, can be treated as posix symlinks only
> + if the SubstituteName can be converted from a native NT
> + object namespace name to a win32 name. We only know how
> + to convert names with two prefixes :
> + "\??\UNC\..."
> + "\??\X:..."
> + Other reparse points will be treated as files or
> + directories, not as posix symlinks. Possible values
> + include:
> + "\??\Volume{..."
> + "\Device\HarddiskVolume1\..."
> + "\Device\Lanman\...\..."
> + */
> + if (RtlEqualUnicodePathPrefix( subst, &ro_u_uncp, TRUE))
> + {
> + return true;
> + }
Please drop the braces for single line branches.
> + else if (RtlEqualUnicodePathPrefix (subst, &ro_u_natp, TRUE) &&
The `else' is gratuitous, the if branch returns anyway.
The case sensitivity parameter can be FALSE here.
> + subst->Length >= 6*sizeof(WCHAR) && subst->Buffer[5] == ':' &&
> + (subst->Length == 6*sizeof(WCHAR) || subst->Buffer[6] == '\\'))
wchar literals should have a leading L, as in L':'.
> + {
> + return true;
> + }
> + return false;
Also, wouldn't it make sense to reorder the code a bit, to avoid the
uncp comparison for local paths, given that most access is local anyway?
Kind of like
if (RtlEqualUnicodePathPrefix (subst, &ro_u_natp, TRUE))
{
if (':' && ('\0' || '\\'))
return true;
if (uncp)
return true;
}
return false;
I think it would be ok to just check for "UNC\\" then, as in
wcsncmp (subst->Buffer + 4, L"UNC\\", 4)
> +}
> +
> +int
> +get_reparse_point_type (PREPARSE_DATA_BUFFER rp, bool remote, PUNICODE_STRING subst)
Same as for readdir_check_reparse_point, I guess this should return
unsigned char.
> +{
> + if (rp->ReparseTag == IO_REPARSE_TAG_SYMLINK)
> + {
> + /* Windows evaluates native symlink literally. If a remote symlink
> + points to, say, C:\foo, it will be handled as if the target is the
> + local file C:\foo. That comes in handy since that's how symlinks
> + are treated under POSIX as well. */
> + RtlInitCountedUnicodeString (subst,
> + (WCHAR *)((char *)rp->SymbolicLinkReparseBuffer.PathBuffer
> + + rp->SymbolicLinkReparseBuffer.SubstituteNameOffset),
> + rp->SymbolicLinkReparseBuffer.SubstituteNameLength);
> + /* Native symlinks are treated as posix symlinks if relative or if the
> + target has a prefix that we know how to deal with.
> + */
> + if ((rp->SymbolicLinkReparseBuffer.Flags & SYMLINK_FLAG_RELATIVE) ||
> + check_reparse_point_target (subst))
> + return DT_LNK;
> + }
> + else if (rp->ReparseTag == IO_REPARSE_TAG_MOUNT_POINT)
> + {
> + RtlInitCountedUnicodeString (subst,
> + (WCHAR *)((char *)rp->MountPointReparseBuffer.PathBuffer
> + + rp->MountPointReparseBuffer.SubstituteNameOffset),
> + rp->MountPointReparseBuffer.SubstituteNameLength);
> + /* Don't handle junctions on remote filesystems as symlinks. This type
> + of reparse point is handled transparently by the OS so that the
> + target of the junction is the remote directory it is supposed to
> + point to. If we handle it as symlink, it will be mistreated as
> + pointing to a dir on the local system. */
> + if (remote)
> + return DT_DIR;
> + /* Native mount points are treated as posix symlinks only if
> + the target has a prefix that does not indicate a volume
> + mount point and that we know how to deal with.
> + */
> + if (check_reparse_point_target (subst))
> + return DT_LNK;
> + }
> + else
> + {
> + /* Maybe it's a reparse point, but it's certainly not one we recognize.
> + */
> + memset (subst, 0, sizeof (*subst));
> + return DT_UNKNOWN;
> + }
> + return DT_DIR;
Same thing with `else' here. Especially the last else branch is
confusing, given that it always leads to a return from the function so
this last line is unreachable code (which Coverity will complain about).
The unassuming reader might wonder why the function defaults to DT_DIR
while in fact it doesn't.
> @@ -2944,22 +3017,25 @@ restart:
> &= ~FILE_ATTRIBUTE_DIRECTORY;
> break;
> }
> - else
> + else if (res == -1)
> {
> - /* Volume moint point or unrecognized reparse point type.
> + /* Volume moint point or unhandled reparse point.
> Make sure the open handle is not used in later stat calls.
> The handle has been opened with the FILE_OPEN_REPARSE_POINT
> flag, so it's a handle to the reparse point, not a handle
> - to the volumes root dir. */
> + to the reparse point target. */
> pflags &= ~PC_KEEP_HANDLE;
> - /* Volume mount point: The filesystem information for the top
> - level directory should be for the volume top level directory,
> - rather than for the reparse point itself. So we fetch the
> - filesystem information again, but with a NULL handle.
> - This does what we want because fs_info::update opens the
> - handle without FILE_OPEN_REPARSE_POINT. */
> - if (res == -1)
> - fs.update (&upath, NULL);
> + /* The filesystem information should be for the target of
> + the reparse point rather than for the reparse point itself.
> + So we fetch the filesystem information again, but with a
> + NULL handle. This does what we want because fs_info::update
> + opens the handle without FILE_OPEN_REPARSE_POINT. */
> + fs.update (&upath, NULL);
> + }
> + else
> + {
> + /* Unknown reparse point type: HSM, dedup, compression, ...
> + Treat as normal directory. */
> }
Nothing against reordering the code, but this drops removing
PC_KEEP_HANDLE from pflags if res == 0, i.e., for unknown reparse
points.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20170612/7f553303/attachment.sig>
More information about the Cygwin-patches
mailing list