[MERGE] sftp transport: do not chmod a dir when unecessary (fix suid and sgid problems).
John Arbash Meinel
john at arbash-meinel.com
Wed Jul 9 02:29:31 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Christophe TROESTLER wrote:
> After discussions with John Arbash Meinel (who I'd like to thank for
> his help) on #bzr, I am submitting this patch.
> The problem is that, with the sftp protocol, any chmod will erase the
> suid or sgid bits. This can be a problem when the sgid bit is used to
> enable sharing among several developers. This patch issues chmod _for
> directories_ only when it is necessary. When chmod is necessary but
> will erase suid/sgid bits, a warning is issued to the user telling him
> to set the proper mask so that chmod is no longer needed.
> There is an extra cost of lstat'ing the file which is usually more or
> less balanced by the fact that chmod is not needed. Moreover, only
> directory (not file) creation is affected.
> P.S. The point of chmod, I was told, is to ensure that the g+w bit is
> set (for sharing). This however does not appears to work when the
> server umask does not allow it. Do we want to warn the user in this
> case as well?
We specifically preserve the bits that were set on the .bzr/ directory. So we
dir_mode = lstat('.bzr/').st_mode
and then files get
file_mode = dir_mode & ~0111 # Remove the exec bit
So if your top level .bzr/ directory doesn't have +w, then none of the
directories underneath will. (After all, you might be pushing to a private
branch, and not want to expose everything.)
The only question is when there isn't a .bzr/ directory to stat. I think we
try to stat its containing directory, but I'm not positive if that is still
As for the patch... I think I would reword the warning a bit:
+ if mode != stat.st_mode & 0777:
+ if stat.st_mode > 01000:
+ warning('The server set a suid or sgid bit on '
+ '%s. If you want to preserve it, use '
+ '"umask 0%03o" on the server.'
+ % (abspath, 0777 - mode))
+ self._get_sftp().chmod(abspath, mode=mode)
First thing I see.. "stat.st_mode > 01000", but usually the fact the object is
a file or directory is stored in the upper bits, so you should be masking
those off. I would probably just do:
if stat.st_mode & 07000:
warning('The server has a suid or sgid bit set on %s.'
' If you want to preserve it, use umask ...')
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar