[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] don't "chmod -x" directories

From: Karl Fogel <kfogel_at_red-bean.com>
Date: 2007-09-17 21:38:07 CEST

"Samuel GĂ©lineau" <gelisam@gmail.com> writes:
> I've found a bug in the subversion client, I have briefly discussed it
> on irc://irc.freenode.net/#svn, I have written a patch and a test
> case. Here is the story of my adventure.
>
> As you probably know, subversion doesn't cope well with files that are
> both "svn:special" and "svn:executable". Making sure this never
> happens is a one line script:
> find -type l | xargs svn propdel svn:executable
>
> However, after doing this, some of my directories were acting weird
> because their executable permission bit was turned off. Now, normally
> "svn propdel svn:executable" only calls "chmod -x" on files, never on
> directories. However, I was now using propdel on _symlinked_
> directories, and this fooled propdel into calling "chmod -x" on them
> anyway.
>
> I didn't want that to happen, so I wrote this patch. Enjoy.
>
> [[
> Prevent "svn propdel svn:executable symlink-to-dir" from doing "chmod
> -x" on the directory
> * subversion-1.4.5/subversion/libsvn_wc/props.c
> (svn_wc_prop_set2): don't drop permissions if the target has svn:special
> ]]

Thanks for the patch!

I think we may need a slightly different solution. Below, I'll
comment on your patch, then propose a different way.

> --- subversion/libsvn_wc/props.c 2006-07-03 04:51:44.000000000 -0400
> +++ subversion/libsvn_wc/props.c 2007-09-11 23:45:41.884118854 -0400
> @@ -1515,6 +1515,13 @@
> SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access,
> svn_path_dirname(path, pool), pool));
>
> + err = svn_wc__load_props(&base_prophash, &prophash, adm_access, base_name,
> + pool);
> + if (err)
> + return
> + svn_error_quick_wrap
> + (err, _("Failed to load properties from disk"));
> +

By the way, today's trunk code has the SVN_ERR_W() macro for this.

> /* Setting an inappropriate property is not allowed (unless
> overridden by 'skip_checks', in some circumstances). Deleting an
> inappropriate property is allowed, however, since older clients
> @@ -1575,7 +1582,10 @@
> in), then chmod -x. */
> if (value == NULL)
> {
> - SVN_ERR(svn_io_set_file_executable(path, FALSE, TRUE, pool));
> + if (!svn_wc__has_special_property(prophash))
> + {
> + SVN_ERR(svn_io_set_file_executable(path, FALSE, TRUE, pool));
> + }
> }

Two problems here:

First, "svn:special" doesn't necessarily mean symlink. Symlink is the
only form of "special" today, but that might not be the case tomorrow.

Second, if the target of the symlink is a file, not a directory, then
do we want to set the executable bit or not? I'm not sure.

However, I thought a better way to address this problem would be to
change svn_io_set_file_executable(), rather than changing its callers.
That function is very short:

   svn_error_t *
   svn_io_set_file_executable(const char *path,
                              svn_boolean_t executable,
                              svn_boolean_t ignore_enoent,
                              apr_pool_t *pool)
   {
     /* On Windows, just exit -- on unix call our internal function
     which attempts to honor the umask. */
   #ifndef WIN32
     return io_set_file_perms(path, FALSE, FALSE, TRUE, executable,
                              ignore_enoent, pool);
   #else
     return SVN_NO_ERROR;
   #endif
   }

All the work happens in io_set_file_perms(). But in that function,
when we call apr_stat() we don't set the APR_FINFO_LINK bit in the
'wanted' mask(), so we don't even know if we're dealing with a link or
not. In other words, today we have this...

   apr_stat(&finfo, path_apr, APR_FINFO_PROT, pool);

...instead of this:

   apr_stat(&finfo, path_apr, APR_FINFO_LINK | APR_FINFO_PROT, pool);

If you print out finfo afterwards, you can see the result:

   (gdb) p finfo
   $4 = { pool = 0x808dc70,
          valid = 7598448,
          protection = 1877,
          filetype = APR_DIR,
          user = 1000,
          group = 1000,
          inode = 9896226,
          device = 2049,
          nlink = 3,
          size = 4096,
          csize = -5191814459127623448,
          atime = 1190056538000000,
          mtime = 1190056539000000,
          ctime = 1190056539000000,
          fname = 0x808b350 "wc/symlink",
          name = 0xb7f11640 "ö\027",
          filehand = 0xb7f8c308 }
   
The filetype is APR_DIR, but the name is "wc/symlink".

I'll post a separate mail about how we can/should change
io_set_file_perms() to protect against this situation. The subject
line will be "Setting x-bit on symlinks to dirs vs files."

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 17 21:38:20 2007

This is an archived mail posted to the Subversion Dev mailing list.