This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Compatibility improvement to reparse point handling, v2


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

Attachment: signature.asc
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]