bug#13125: Fix permissions bugs with setgid directories etc.

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

Tags: patch security

Emacs sometimes mishandles the permissions of files: even if
backup-by-copying-when-mismatch is set, Emacs sometimes replaces a
rewritten file with a file that has the wrong user or group.

Here's some background.

In several places Emacs assumes that on 4.2BSD hosts, a newly created
file is given a group ID equal to its parent directory, and that on
non-4.2BSD hosts the new files are given Emacs's group ID.  Although
this was true long ago, it hasn't been true for many years.  Most
commonly, the old 4.2BSD behavior is now selected by the setgid bit on
directories.  But on some hosts, the behavior is selected as a mount
flag, or (as in 4.2BSD) it's a property of the operating system.  On
network file systems the behavior is sometimes selected by the file
server, sometimes by the client.

To add to the mess, on FreeBSD systems, the setuid bit of directories
can control whether there's a similar inheritance of file ownership.
Luckily this problem is a bit simpler, in that it's not a property
of the OS or a mount flag, as far as I know.

I'm attaching a patch, which changes file-attributes so that it now
outputs a placeholder value instead of the old 9th attribute member,
since the value is rarely needed and almost nobody seems to be using
it or caring that it's wrong.  Instead, the patch moves this
functionality to file-ownership-preserved-p via a new argument GROUP.
The patch also adds new functions group-gid and group-real-gid for use
with the backup-file heuristic.

This patch is relative to trunk bzr 111160.

[-- Attachment #2: setgiddir.txt --]
[-- Type: text/plain, Size: 16454 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2012-12-08 17:19:51 +0000
+++ ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,8 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* configure.ac (BSD4_2): Remove; no longer needed.
+
 2012-12-08  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Use putenv+unsetenv instead of modifying environ directly (Bug#13070).

=== modified file 'admin/CPP-DEFINES'
--- admin/CPP-DEFINES	2012-12-08 06:56:26 +0000
+++ admin/CPP-DEFINES	2012-12-09 00:50:02 +0000
@@ -9,7 +9,6 @@
 
 AIX
 _AIX
-BSD4_2
 BSD_SYSTEM
 CYGWIN		Compiling the Cygwin port.
 __CYGWIN__	Ditto

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-12-08 17:19:51 +0000
+++ admin/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,8 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* CPP-DEFINES (BSD4_2): Remove.
+
 2012-12-08  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Use putenv+unsetenv instead of modifying environ directly (Bug#13070).

=== modified file 'configure.ac'
--- configure.ac	2012-12-08 06:56:26 +0000
+++ configure.ac	2012-12-09 00:50:02 +0000
@@ -3814,7 +3814,6 @@
 
 dnl Define symbols to identify the version of Unix this is.
 dnl Define all the symbols that apply correctly.
-AH_TEMPLATE(BSD4_2, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(BSD_SYSTEM, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(DOS_NT, [Define if the system is MS DOS or MS Windows.])
 AH_TEMPLATE(MSDOS, [Define if the system is MS DOS.])
@@ -3840,7 +3839,6 @@
 
   darwin)
     dnl BSD4_3 and BSD4_4 are already defined in sys/param.h.
-    AC_DEFINE(BSD4_2, [])
     AC_DEFINE(BSD_SYSTEM, [])
     dnl More specific than the above two.  We cannot use __APPLE__ as this
     dnl may not be defined on non-OSX Darwin, and we cannot define DARWIN
@@ -3850,7 +3848,6 @@
     ;;
 
   freebsd)
-    AC_DEFINE(BSD4_2, [])
     dnl Hack to avoid calling AC_PREPROC_IFELSE multiple times.
     dnl Would not be needed with autoconf >= 2.67, where the
     dnl preprocessed output is accessible in "conftest.i".
@@ -3858,7 +3855,6 @@
     ;;
 
   gnu | netbsd | openbsd )
-    AC_DEFINE(BSD4_2, [])
     AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[
 #ifndef BSD_SYSTEM
 # error "BSD_SYSTEM not defined"

=== modified file 'doc/lispintro/ChangeLog'
--- doc/lispintro/ChangeLog	2012-12-06 08:33:32 +0000
+++ doc/lispintro/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,10 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* emacs-lisp-intro.texi (Files List):
+	directory-files-and-attributes now outputs 0 instead of t for
+	attribute that's now a placeholder.
+
 2012-12-06  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* doclicense.texi: Update to latest version from FSF.

=== modified file 'doc/lispintro/emacs-lisp-intro.texi'
--- doc/lispintro/emacs-lisp-intro.texi	2012-12-05 22:27:56 +0000
+++ doc/lispintro/emacs-lisp-intro.texi	2012-12-09 00:50:02 +0000
@@ -15687,7 +15687,7 @@
 "-rw-r--r--"
 @end group
 @group
-nil
+0
 2971624
 773)
 @end group

=== modified file 'doc/lispref/ChangeLog'
--- doc/lispref/ChangeLog	2012-12-06 08:33:32 +0000
+++ doc/lispref/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,13 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* files.texi (Testing Accessibility): Document GROUP arg
+	of file-ownership-preserved-p.
+	(File Attributes): Document that 9th element is now
+	just a placeholder.
+	* os.texi (User Identification): Document new functions group-gid,
+	group-real-gid.
+
 2012-12-06  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* doclicense.texi, gpl.texi: Update to latest version from FSF.

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2012-12-05 22:27:56 +0000
+++ doc/lispref/files.texi	2012-12-09 00:50:02 +0000
@@ -895,11 +895,14 @@
 using @var{string} as the error message text.
 @end defun
 
-@defun file-ownership-preserved-p filename
+@defun file-ownership-preserved-p filename &optional group
 This function returns @code{t} if deleting the file @var{filename} and
 then creating it anew would keep the file's owner unchanged.  It also
 returns @code{t} for nonexistent files.
 
+If the optional argument @var{group} is non-@code{nil}, this function
+also checks that the file's group would be unchanged.
+
 If @var{filename} is a symbolic link, then, unlike the other functions
 discussed here, @code{file-ownership-preserved-p} does @emph{not}
 replace @var{filename} with its target.  However, it does recursively
@@ -1246,8 +1249,7 @@
 as in @samp{ls -l}.
 
 @item
-@code{t} if the file's @acronym{GID} would change if file were
-deleted and recreated; @code{nil} otherwise.
+An unspecified value, present for backward compatibility.
 
 @item
 The file's inode number.  If possible, this is an integer.  If the
@@ -1279,7 +1281,7 @@
           (20000 23 0 0)
           (20614 64555 902289 872000)
           122295 "-rw-rw-rw-"
-          nil  (5888 2 . 43978)
+          0  (5888 2 . 43978)
           (15479 . 46724))
 @end group
 @end example
@@ -1318,8 +1320,8 @@
 @item "-rw-rw-rw-"
 has a mode of read and write access for the owner, group, and world.
 
-@item nil
-would retain the same @acronym{GID} if it were recreated.
+@item 0
+is merely a placeholder; it carries no information.
 
 @item (5888 2 . 43978)
 has an inode number of 6473924464520138.

=== modified file 'doc/lispref/os.texi'
--- doc/lispref/os.texi	2012-12-06 06:17:10 +0000
+++ doc/lispref/os.texi	2012-12-09 00:50:02 +0000
@@ -1157,6 +1157,16 @@
 The value may be a floating point number.
 @end defun
 
+@defun group-gid
+This function returns the effective @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
+@defun group-real-gid
+This function returns the real @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
 @defun system-users
 This function returns a list of strings, listing the user names on the
 system.  If Emacs cannot retrieve this information, the return value

=== modified file 'etc/ChangeLog'
--- etc/ChangeLog	2012-12-06 22:44:05 +0000
+++ etc/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,10 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* NEWS: Document changes to file-attributes,
+	file-ownership-preserved-p.
+	Mention new functions group-gid, group-real-gid.
+
 2012-12-06  Andreas Schwab  <schwab@linux-m68k.org>
 
 	* themes/leuven-theme.el: Convert to Unix format.

=== modified file 'etc/NEWS'
--- etc/NEWS	2012-12-07 04:37:14 +0000
+++ etc/NEWS	2012-12-09 00:50:02 +0000
@@ -142,6 +142,17 @@
 ** The `defalias-fset-function' property lets you catch calls to defalias
 and redirect them to your own function instead of `fset'.
 
+** The 9th element returned by `file-attributes' is now unspecified.
+Formerly, it was t if the file's gid would change if file were deleted
+and recreated.  This value has been inaccurate for years on many
+platforms, and nobody seems to have noticed or cared.
+
+** The function `file-ownership-preserved-p' now has an optional
+argument GROUP which causes it check for file group too.  This can be
+used in place of the 9th element of `file-attributes'.
+
+** New functions `group-gid' and `group-real-gid'.
+
 * Changes in Emacs 24.4 on non-free operating systems
 
 +++

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-08 23:12:08 +0000
+++ lisp/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,19 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* files.el (backup-buffer): Don't rely on 9th output of
+	file-attributes, as it's now a placeholder.  Instead, use the new
+	optional arg of file-ownership-preserved-p.
+	(file-ownership-preserved-p): New optional arg GROUP.
+	Fix mishandling of setuid directories that would cause this
+	function to return t when it should have returned nil.
+	Document what happens if the file does not exist, and when
+	it's not known whether the ownership will be preserved.
+	* net/tramp-sh.el (tramp-sh-handle-file-ownership-preserved-p):
+	Likewise.
+	(tramp-get-local-gid): Use group-gid for integer, as that's
+	faster and more reliable.
+
 2012-12-08  Juri Linkov  <juri@jurta.org>
 
 	* info.el (Info-copy-current-node-name, Info-breadcrumbs)

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-03 01:08:31 +0000
+++ lisp/files.el	2012-12-09 00:50:02 +0000
@@ -3941,8 +3941,8 @@
 					      (and (integerp (nth 2 attr))
 						   (integerp backup-by-copying-when-privileged-mismatch)
 						   (<= (nth 2 attr) backup-by-copying-when-privileged-mismatch)))
-					  (or (nth 9 attr)
-					      (not (file-ownership-preserved-p real-file-name)))))))
+					  (not (file-ownership-preserved-p
+						real-file-name t))))))
 			  (backup-buffer-copy real-file-name backupname modes context)
 			;; rename-file should delete old backup.
 			(rename-file real-file-name backupname t)
@@ -4019,22 +4019,40 @@
                    (string-match (concat file-name-version-regexp "\\'")
                                  name))))))
 
-(defun file-ownership-preserved-p (file)
-  "Return t if deleting FILE and rewriting it would preserve the owner."
+(defun file-ownership-preserved-p (file &optional group)
+  "Return t if deleting FILE and rewriting it would preserve the owner.
+Return nil if FILE does not exist, or if deleting and recreating it
+might not preserve the owner.  If GROUP is non-nil, check whether
+the group would be preserved too."
   (let ((handler (find-file-name-handler file 'file-ownership-preserved-p)))
     (if handler
-	(funcall handler 'file-ownership-preserved-p file)
+	(funcall handler 'file-ownership-preserved-p file group)
       (let ((attributes (file-attributes file 'integer)))
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (user-uid))
-	    ;; Files created on Windows by Administrator (RID=500)
-	    ;; have the Administrators group (RID=544) recorded as
-	    ;; their owner.  Rewriting them will still preserve the
-	    ;; owner.
-	    (and (eq system-type 'windows-nt)
-		 (= (user-uid) 500) (= (nth 2 attributes) 544)))))))
+	    (and (or (= (nth 2 attributes) (user-uid))
+		     ;; Files created on Windows by Administrator (RID=500)
+		     ;; have the Administrators group (RID=544) recorded as
+		     ;; their owner.  Rewriting them will still preserve the
+		     ;; owner.
+		     (and (eq system-type 'windows-nt)
+			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
+		 (or (not group)
+		     (= (nth 3 attributes) (group-gid)))
+		 (let* ((parent (or (file-name-directory file) "."))
+			(parent-attributes (file-attributes parent 'integer)))
+		   (and parent-attributes
+			;; On some systems, a file created in a setuid directory
+			;; inherits that directory's owner.
+			(or
+			 (= (nth 2 parent-attributes) (user-uid))
+			 (string-match "^...[^sS]" (nth 8 parent-attributes)))
+			;; On many systems, a file created in a setgid directory
+			;; inherits that directory's group.  On some systems
+			;; this happens even if the setgid bit is not set.
+			(or (not group)
+			    (= (nth 3 parent-attributes) (group-gid)))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

=== modified file 'lisp/net/tramp-sh.el'
--- lisp/net/tramp-sh.el	2012-12-06 09:15:27 +0000
+++ lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
@@ -1616,7 +1616,7 @@
 	(and (tramp-run-test "-d" (file-name-directory filename))
 	     (tramp-run-test "-w" (file-name-directory filename)))))))
 
-(defun tramp-sh-handle-file-ownership-preserved-p (filename)
+(defun tramp-sh-handle-file-ownership-preserved-p (filename &optional group)
   "Like `file-ownership-preserved-p' for Tramp files."
   (with-parsed-tramp-file-name filename nil
     (with-tramp-file-property v localname "file-ownership-preserved-p"
@@ -1624,7 +1624,10 @@
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (tramp-get-remote-uid v 'integer)))))))
+	    (and
+	     (= (nth 2 attributes) (tramp-get-remote-uid v 'integer))
+	     (or (not group)
+		 (= (nth 3 attributes) (tramp-get-remote-gid v 'integer)))))))))
 
 ;; Directory listings.
 
@@ -5021,7 +5024,9 @@
   (if (equal id-format 'integer) (user-uid) (user-login-name)))
 
 (defun tramp-get-local-gid (id-format)
-  (nth 3 (tramp-compat-file-attributes "~/" id-format)))
+  (if (equal id-format 'integer)
+      (group-gid)
+    (nth 3 (tramp-compat-file-attributes "~/" id-format))))
 
 ;; Some predefined connection properties.
 (defun tramp-get-inline-compress (vec prop size)

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-12-08 18:27:37 +0000
+++ src/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,12 @@
+2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc.
+	* dired.c (Ffile_attributes): Return 0 as the 9th attribute,
+	to mark it as a placeholder.  The old value was often wrong.
+	The only user of this attribute has been changed to use
+	file-ownership-preserved-p instead, with its new group arg.
+	* editfns.c (Fgroup_gid, Fgroup_real_gid): New functions.
+
 2012-12-08  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32.c (unsetenv): Return 0 if the input string is too long.

=== modified file 'src/dired.c'
--- src/dired.c	2012-11-27 05:38:42 +0000
+++ src/dired.c	2012-12-09 00:50:02 +0000
@@ -869,7 +869,7 @@
  7. Size in bytes.
   This is a floating point number if the size is too large for an integer.
  8. File modes, as a string of ten letters or dashes as in ls -l.
- 9. t if file's gid would change if file were deleted and recreated.
+ 9. An unspecified value, present only for backward compatibility.
 10. inode number.  If it is larger than what an Emacs integer can hold,
   this is of the form (HIGH . LOW): first the high bits, then the low 16 bits.
   If even HIGH is too large for an Emacs integer, this is instead of the form
@@ -891,10 +891,6 @@
   Lisp_Object values[12];
   Lisp_Object encoded;
   struct stat s;
-#ifdef BSD4_2
-  Lisp_Object dirname;
-  struct stat sdir;
-#endif /* BSD4_2 */
 
   /* An array to hold the mode string generated by filemodestring,
      including its terminating space and null byte.  */
@@ -959,17 +955,7 @@
 
   filemodestring (&s, modes);
   values[8] = make_string (modes, 10);
-#ifdef BSD4_2 /* file gid will be dir gid */
-  dirname = Ffile_name_directory (filename);
-  if (! NILP (dirname))
-    encoded = ENCODE_FILE (dirname);
-  if (! NILP (dirname) && stat (SDATA (encoded), &sdir) == 0)
-    values[9] = (sdir.st_gid != s.st_gid) ? Qt : Qnil;
-  else					/* if we can't tell, assume worst */
-    values[9] = Qt;
-#else					/* file gid will be egid */
-  values[9] = (s.st_gid != getegid ()) ? Qt : Qnil;
-#endif	/* not BSD4_2 */
+  values[9] = make_number (0);
   values[10] = INTEGER_TO_CONS (s.st_ino);
   values[11] = INTEGER_TO_CONS (s.st_dev);
 

=== modified file 'src/editfns.c'
--- src/editfns.c	2012-12-08 17:19:51 +0000
+++ src/editfns.c	2012-12-09 00:50:02 +0000
@@ -1272,6 +1272,24 @@
   return make_fixnum_or_float (uid);
 }
 
+DEFUN ("group-gid", Fgroup_gid, Sgroup_gid, 0, 0, 0,
+       doc: /* Return the effective gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t egid = getegid ();
+  return make_fixnum_or_float (egid);
+}
+
+DEFUN ("group-real-gid", Fgroup_real_gid, Sgroup_real_gid, 0, 0, 0,
+       doc: /* Return the real gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t gid = getgid ();
+  return make_fixnum_or_float (gid);
+}
+
 DEFUN ("user-full-name", Fuser_full_name, Suser_full_name, 0, 1, 0,
        doc: /* Return the full name of the user logged in, as a string.
 If the full name corresponding to Emacs's userid is not known,
@@ -4899,6 +4917,8 @@
   defsubr (&Suser_real_login_name);
   defsubr (&Suser_uid);
   defsubr (&Suser_real_uid);
+  defsubr (&Sgroup_gid);
+  defsubr (&Sgroup_real_gid);
   defsubr (&Suser_full_name);
   defsubr (&Semacs_pid);
   defsubr (&Scurrent_time);

bug#13125: Fix permissions bugs with setgid directories etc.

[Chong Yidong]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I'm attaching a patch, which changes file-attributes so that it now
> outputs a placeholder value instead of the old 9th attribute member,
> since the value is rarely needed and almost nobody seems to be using
                     ^^^^^^

Have you actually done a survey of all the callers to file-attributes to
see if this is true?

> Instead, the patch moves this functionality to
> file-ownership-preserved-p via a new argument GROUP.

Why can't this functionality be kept in file-attributes?

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

On 12/08/2012 07:54 PM, Chong Yidong wrote:
> Have you actually done a survey of all the callers to file-attributes to
> see if this is true?

Yes, all the callers that are distributed as part of Emacs.

>> > Instead, the patch moves this functionality to
>> > file-ownership-preserved-p via a new argument GROUP.
> Why can't this functionality be kept in file-attributes?

Performance.  The functionality requires statting the parent directory
for each call to file-attributes.

bug#13125: Fix permissions bugs with setgid directories etc.

[Eli Zaretskii]

> Date: Sat, 08 Dec 2012 23:26:27 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: 13125@debbugs.gnu.org
> 
> On 12/08/2012 07:54 PM, Chong Yidong wrote:
> > Have you actually done a survey of all the callers to file-attributes to
> > see if this is true?
> 
> Yes, all the callers that are distributed as part of Emacs.

Did you find _any_ of them that even reference this attribute?  I
didn't, but perhaps I missed something.

Given the "wide" use, it is hard to reason what should be the value of
this attribute after these changes are installed.  You set them to
zero, which is neither nil nor t; thus, code that was testing for
either of these two values explicitly will now fail, while code that
was testing for non-nil will succeed where perhaps it shouldn't have.
For these reasons, I would suggest to leave the value at one of these
two, whichever is more frequent in real life.

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]

On 12/09/2012 09:03 AM, Eli Zaretskii wrote:

> Did you find _any_ of them that even reference this attribute?

Yes, just one: backup-buffer.  It's fixed in the proposed patch,
in the first hunk of the lisp/files.el patch.

> Given the "wide" use, it is hard to reason what should be the value of
> this attribute after these changes are installed.  You set them to
> zero, which is neither nil nor t; thus, code that was testing for
> either of these two values explicitly will now fail, while code that
> was testing for non-nil will succeed where perhaps it shouldn't have.

The only example I found, in backup-buffer, was testing for non-nil.
Zero counts as non-nil, so if backup-buffer were not changed, it'd be
treating the value as t.  This would be safe, as it's the nil case
that is dangerous.  (With the further change below, this paragraph is
moot.)

> I would suggest to leave the value at one of these
> two, whichever is more frequent in real life.

If we were to leave the value as one of these two, we should leave it
as t, the safer value.  Here's a further patch to do that, and I'll
attach the updated combined patch (integrating all the further patches
suggested so far), relative to trunk bzr 111167.

=== modified file 'doc/lispintro/ChangeLog'
--- doc/lispintro/ChangeLog	2012-12-09 02:30:06 +0000
+++ doc/lispintro/ChangeLog	2012-12-10 00:56:35 +0000
@@ -1,9 +1,9 @@
-2012-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Fix permissions bugs with setgid directories etc. (Bug#13125)
 	* emacs-lisp-intro.texi (Files List):
-	directory-files-and-attributes now outputs 0 instead of t for
-	attribute that's now a placeholder.
+	directory-files-and-attributes now outputs t for attribute that's
+	now a placeholder.
 
 2012-12-06  Paul Eggert  <eggert@cs.ucla.edu>
 

=== modified file 'doc/lispintro/emacs-lisp-intro.texi'
--- doc/lispintro/emacs-lisp-intro.texi	2012-12-09 00:50:02 +0000
+++ doc/lispintro/emacs-lisp-intro.texi	2012-12-10 00:56:35 +0000
@@ -15687,7 +15687,7 @@
 "-rw-r--r--"
 @end group
 @group
-0
+t
 2971624
 773)
 @end group

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2012-12-09 00:50:02 +0000
+++ doc/lispref/files.texi	2012-12-10 00:56:35 +0000
@@ -1281,7 +1281,7 @@
           (20000 23 0 0)
           (20614 64555 902289 872000)
           122295 "-rw-rw-rw-"
-          0  (5888 2 . 43978)
+          t (5888 2 . 43978)
           (15479 . 46724))
 @end group
 @end example
@@ -1320,7 +1320,7 @@
 @item "-rw-rw-rw-"
 has a mode of read and write access for the owner, group, and world.
 
-@item 0
+@item t
 is merely a placeholder; it carries no information.
 
 @item (5888 2 . 43978)

=== modified file 'src/dired.c'
--- src/dired.c	2012-12-09 00:50:02 +0000
+++ src/dired.c	2012-12-10 00:56:35 +0000
@@ -955,7 +955,7 @@
 
   filemodestring (&s, modes);
   values[8] = make_string (modes, 10);
-  values[9] = make_number (0);
+  values[9] = Qt;
   values[10] = INTEGER_TO_CONS (s.st_ino);
   values[11] = INTEGER_TO_CONS (s.st_dev);
 


[-- Attachment #2: setgiddir.txt --]
[-- Type: text/plain, Size: 16725 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2012-12-09 08:37:01 +0000
+++ ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,8 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* configure.ac (BSD4_2): Remove; no longer needed.
+
 2012-12-09  Andreas Schwab  <schwab@linux-m68k.org>
 
 	* configure.ac: Fix source command in .gdbinit.

=== modified file 'admin/CPP-DEFINES'
--- admin/CPP-DEFINES	2012-12-08 06:56:26 +0000
+++ admin/CPP-DEFINES	2012-12-09 00:50:02 +0000
@@ -9,7 +9,6 @@
 
 AIX
 _AIX
-BSD4_2
 BSD_SYSTEM
 CYGWIN		Compiling the Cygwin port.
 __CYGWIN__	Ditto

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-12-08 17:19:51 +0000
+++ admin/ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,8 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* CPP-DEFINES (BSD4_2): Remove.
+
 2012-12-08  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Use putenv+unsetenv instead of modifying environ directly (Bug#13070).

=== modified file 'configure.ac'
--- configure.ac	2012-12-09 08:37:01 +0000
+++ configure.ac	2012-12-09 23:05:24 +0000
@@ -3819,7 +3819,6 @@
 
 dnl Define symbols to identify the version of Unix this is.
 dnl Define all the symbols that apply correctly.
-AH_TEMPLATE(BSD4_2, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(BSD_SYSTEM, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(DOS_NT, [Define if the system is MS DOS or MS Windows.])
 AH_TEMPLATE(MSDOS, [Define if the system is MS DOS.])
@@ -3845,7 +3844,6 @@
 
   darwin)
     dnl BSD4_3 and BSD4_4 are already defined in sys/param.h.
-    AC_DEFINE(BSD4_2, [])
     AC_DEFINE(BSD_SYSTEM, [])
     dnl More specific than the above two.  We cannot use __APPLE__ as this
     dnl may not be defined on non-OSX Darwin, and we cannot define DARWIN
@@ -3855,7 +3853,6 @@
     ;;
 
   freebsd)
-    AC_DEFINE(BSD4_2, [])
     dnl Hack to avoid calling AC_PREPROC_IFELSE multiple times.
     dnl Would not be needed with autoconf >= 2.67, where the
     dnl preprocessed output is accessible in "conftest.i".
@@ -3863,7 +3860,6 @@
     ;;
 
   gnu | netbsd | openbsd )
-    AC_DEFINE(BSD4_2, [])
     AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[
 #ifndef BSD_SYSTEM
 # error "BSD_SYSTEM not defined"

=== modified file 'doc/lispintro/ChangeLog'
--- doc/lispintro/ChangeLog	2012-12-06 08:33:32 +0000
+++ doc/lispintro/ChangeLog	2012-12-10 00:56:35 +0000
@@ -1,3 +1,10 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* emacs-lisp-intro.texi (Files List):
+	directory-files-and-attributes now outputs t for attribute that's
+	now a placeholder.
+
 2012-12-06  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* doclicense.texi: Update to latest version from FSF.

=== modified file 'doc/lispintro/emacs-lisp-intro.texi'
--- doc/lispintro/emacs-lisp-intro.texi	2012-12-05 22:27:56 +0000
+++ doc/lispintro/emacs-lisp-intro.texi	2012-12-10 00:56:35 +0000
@@ -15687,7 +15687,7 @@
 "-rw-r--r--"
 @end group
 @group
-nil
+t
 2971624
 773)
 @end group

=== modified file 'doc/lispref/ChangeLog'
--- doc/lispref/ChangeLog	2012-12-09 01:04:43 +0000
+++ doc/lispref/ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,13 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* files.texi (Testing Accessibility): Document GROUP arg
+	of file-ownership-preserved-p.
+	(File Attributes): Document that 9th element is now
+	just a placeholder.
+	* os.texi (User Identification): Document new functions group-gid,
+	group-real-gid.
+
 2012-12-09  Glenn Morris  <rgm@gnu.org>
 
 	* customize.texi (Variable Definitions): Mention eval-defun

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2012-12-05 22:27:56 +0000
+++ doc/lispref/files.texi	2012-12-10 00:56:35 +0000
@@ -895,11 +895,14 @@
 using @var{string} as the error message text.
 @end defun
 
-@defun file-ownership-preserved-p filename
+@defun file-ownership-preserved-p filename &optional group
 This function returns @code{t} if deleting the file @var{filename} and
 then creating it anew would keep the file's owner unchanged.  It also
 returns @code{t} for nonexistent files.
 
+If the optional argument @var{group} is non-@code{nil}, this function
+also checks that the file's group would be unchanged.
+
 If @var{filename} is a symbolic link, then, unlike the other functions
 discussed here, @code{file-ownership-preserved-p} does @emph{not}
 replace @var{filename} with its target.  However, it does recursively
@@ -1246,8 +1249,7 @@
 as in @samp{ls -l}.
 
 @item
-@code{t} if the file's @acronym{GID} would change if file were
-deleted and recreated; @code{nil} otherwise.
+An unspecified value, present for backward compatibility.
 
 @item
 The file's inode number.  If possible, this is an integer.  If the
@@ -1279,7 +1281,7 @@
           (20000 23 0 0)
           (20614 64555 902289 872000)
           122295 "-rw-rw-rw-"
-          nil  (5888 2 . 43978)
+          t (5888 2 . 43978)
           (15479 . 46724))
 @end group
 @end example
@@ -1318,8 +1320,8 @@
 @item "-rw-rw-rw-"
 has a mode of read and write access for the owner, group, and world.
 
-@item nil
-would retain the same @acronym{GID} if it were recreated.
+@item t
+is merely a placeholder; it carries no information.
 
 @item (5888 2 . 43978)
 has an inode number of 6473924464520138.

=== modified file 'doc/lispref/os.texi'
--- doc/lispref/os.texi	2012-12-06 06:17:10 +0000
+++ doc/lispref/os.texi	2012-12-09 00:50:02 +0000
@@ -1157,6 +1157,16 @@
 The value may be a floating point number.
 @end defun
 
+@defun group-gid
+This function returns the effective @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
+@defun group-real-gid
+This function returns the real @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
 @defun system-users
 This function returns a list of strings, listing the user names on the
 system.  If Emacs cannot retrieve this information, the return value

=== modified file 'etc/ChangeLog'
--- etc/ChangeLog	2012-12-06 22:44:05 +0000
+++ etc/ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,10 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* NEWS: Document changes to file-attributes,
+	file-ownership-preserved-p.
+	Mention new functions group-gid, group-real-gid.
+
 2012-12-06  Andreas Schwab  <schwab@linux-m68k.org>
 
 	* themes/leuven-theme.el: Convert to Unix format.

=== modified file 'etc/NEWS'
--- etc/NEWS	2012-12-09 03:40:09 +0000
+++ etc/NEWS	2012-12-09 23:05:24 +0000
@@ -151,6 +151,17 @@
 ** The `defalias-fset-function' property lets you catch calls to defalias
 and redirect them to your own function instead of `fset'.
 
+** The 9th element returned by `file-attributes' is now unspecified.
+Formerly, it was t if the file's gid would change if file were deleted
+and recreated.  This value has been inaccurate for years on many
+platforms, and nobody seems to have noticed or cared.
+
+** The function `file-ownership-preserved-p' now has an optional
+argument GROUP which causes it check for file group too.  This can be
+used in place of the 9th element of `file-attributes'.
+
+** New functions `group-gid' and `group-real-gid'.
+
 * Changes in Emacs 24.4 on non-free operating systems
 
 +++

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-09 21:32:12 +0000
+++ lisp/ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,19 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* files.el (backup-buffer): Don't rely on 9th output of
+	file-attributes, as it's now a placeholder.  Instead, use the new
+	optional arg of file-ownership-preserved-p.
+	(file-ownership-preserved-p): New optional arg GROUP.
+	Fix mishandling of setuid directories that would cause this
+	function to return t when it should have returned nil.
+	Document what happens if the file does not exist, and when
+	it's not known whether the ownership will be preserved.
+	* net/tramp-sh.el (tramp-sh-handle-file-ownership-preserved-p):
+	Likewise.
+	(tramp-get-local-gid): Use group-gid for integer, as that's
+	faster and more reliable.
+
 2012-12-09  Eli Zaretskii  <eliz@gnu.org>
 
 	Parallelize byte compilation on MS-Windows.

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-03 01:08:31 +0000
+++ lisp/files.el	2012-12-10 00:38:45 +0000
@@ -3941,8 +3941,8 @@
 					      (and (integerp (nth 2 attr))
 						   (integerp backup-by-copying-when-privileged-mismatch)
 						   (<= (nth 2 attr) backup-by-copying-when-privileged-mismatch)))
-					  (or (nth 9 attr)
-					      (not (file-ownership-preserved-p real-file-name)))))))
+					  (not (file-ownership-preserved-p
+						real-file-name t))))))
 			  (backup-buffer-copy real-file-name backupname modes context)
 			;; rename-file should delete old backup.
 			(rename-file real-file-name backupname t)
@@ -4019,22 +4019,44 @@
                    (string-match (concat file-name-version-regexp "\\'")
                                  name))))))
 
-(defun file-ownership-preserved-p (file)
-  "Return t if deleting FILE and rewriting it would preserve the owner."
+(defun file-ownership-preserved-p (file &optional group)
+  "Return t if deleting FILE and rewriting it would preserve the owner.
+Return nil if FILE does not exist, or if deleting and recreating it
+might not preserve the owner.  If GROUP is non-nil, check whether
+the group would be preserved too."
   (let ((handler (find-file-name-handler file 'file-ownership-preserved-p)))
     (if handler
-	(funcall handler 'file-ownership-preserved-p file)
+	(funcall handler 'file-ownership-preserved-p file group)
       (let ((attributes (file-attributes file 'integer)))
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (user-uid))
-	    ;; Files created on Windows by Administrator (RID=500)
-	    ;; have the Administrators group (RID=544) recorded as
-	    ;; their owner.  Rewriting them will still preserve the
-	    ;; owner.
-	    (and (eq system-type 'windows-nt)
-		 (= (user-uid) 500) (= (nth 2 attributes) 544)))))))
+	    (and (or (= (nth 2 attributes) (user-uid))
+		     ;; Files created on Windows by Administrator (RID=500)
+		     ;; have the Administrators group (RID=544) recorded as
+		     ;; their owner.  Rewriting them will still preserve the
+		     ;; owner.
+		     (and (eq system-type 'windows-nt)
+			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
+		 (or (not group)
+		     ;; On BSD-derived systems files always inherit the parent
+		     ;; directory's group, so skip the group-gid test.
+		     (memq system-type '(berkeley-unix darwin gnu/kfreebsd))
+		     (= (nth 3 attributes) (group-gid)))
+		 (let* ((parent (or (file-name-directory file) "."))
+			(parent-attributes (file-attributes parent 'integer)))
+		   (and parent-attributes
+			;; On some systems, a file created in a setuid directory
+			;; inherits that directory's owner.
+			(or
+			 (= (nth 2 parent-attributes) (user-uid))
+			 (string-match "^...[^sS]" (nth 8 parent-attributes)))
+			;; On many systems, a file created in a setgid directory
+			;; inherits that directory's group.  On some systems
+			;; this happens even if the setgid bit is not set.
+			(or (not group)
+			    (= (nth 3 parent-attributes)
+			       (nth 3 attributes)))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

=== modified file 'lisp/net/tramp-sh.el'
--- lisp/net/tramp-sh.el	2012-12-06 09:15:27 +0000
+++ lisp/net/tramp-sh.el	2012-12-09 08:53:55 +0000
@@ -1616,7 +1616,7 @@
 	(and (tramp-run-test "-d" (file-name-directory filename))
 	     (tramp-run-test "-w" (file-name-directory filename)))))))
 
-(defun tramp-sh-handle-file-ownership-preserved-p (filename)
+(defun tramp-sh-handle-file-ownership-preserved-p (filename &optional group)
   "Like `file-ownership-preserved-p' for Tramp files."
   (with-parsed-tramp-file-name filename nil
     (with-tramp-file-property v localname "file-ownership-preserved-p"
@@ -1624,7 +1624,10 @@
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (tramp-get-remote-uid v 'integer)))))))
+	    (and
+	     (= (nth 2 attributes) (tramp-get-remote-uid v 'integer))
+	     (or (not group)
+		 (= (nth 3 attributes) (tramp-get-remote-gid v 'integer)))))))))
 
 ;; Directory listings.
 
@@ -5021,7 +5024,9 @@
   (if (equal id-format 'integer) (user-uid) (user-login-name)))
 
 (defun tramp-get-local-gid (id-format)
-  (nth 3 (tramp-compat-file-attributes "~/" id-format)))
+  (if (and (fboundp 'group-gid) (equal id-format 'integer))
+      (tramp-compat-funcall 'group-gid)
+    (nth 3 (tramp-compat-file-attributes "~/" id-format))))
 
 ;; Some predefined connection properties.
 (defun tramp-get-inline-compress (vec prop size)

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-12-08 18:27:37 +0000
+++ src/ChangeLog	2012-12-10 00:59:51 +0000
@@ -1,3 +1,12 @@
+2012-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix permissions bugs with setgid directories etc. (Bug#13125)
+	* dired.c (Ffile_attributes): Return t as the 9th attribute,
+	to mark it as a placeholder.  The old value was often wrong.
+	The only user of this attribute has been changed to use
+	file-ownership-preserved-p instead, with its new group arg.
+	* editfns.c (Fgroup_gid, Fgroup_real_gid): New functions.
+
 2012-12-08  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32.c (unsetenv): Return 0 if the input string is too long.

=== modified file 'src/dired.c'
--- src/dired.c	2012-11-27 05:38:42 +0000
+++ src/dired.c	2012-12-10 00:56:35 +0000
@@ -869,7 +869,7 @@
  7. Size in bytes.
   This is a floating point number if the size is too large for an integer.
  8. File modes, as a string of ten letters or dashes as in ls -l.
- 9. t if file's gid would change if file were deleted and recreated.
+ 9. An unspecified value, present only for backward compatibility.
 10. inode number.  If it is larger than what an Emacs integer can hold,
   this is of the form (HIGH . LOW): first the high bits, then the low 16 bits.
   If even HIGH is too large for an Emacs integer, this is instead of the form
@@ -891,10 +891,6 @@
   Lisp_Object values[12];
   Lisp_Object encoded;
   struct stat s;
-#ifdef BSD4_2
-  Lisp_Object dirname;
-  struct stat sdir;
-#endif /* BSD4_2 */
 
   /* An array to hold the mode string generated by filemodestring,
      including its terminating space and null byte.  */
@@ -959,17 +955,7 @@
 
   filemodestring (&s, modes);
   values[8] = make_string (modes, 10);
-#ifdef BSD4_2 /* file gid will be dir gid */
-  dirname = Ffile_name_directory (filename);
-  if (! NILP (dirname))
-    encoded = ENCODE_FILE (dirname);
-  if (! NILP (dirname) && stat (SDATA (encoded), &sdir) == 0)
-    values[9] = (sdir.st_gid != s.st_gid) ? Qt : Qnil;
-  else					/* if we can't tell, assume worst */
-    values[9] = Qt;
-#else					/* file gid will be egid */
-  values[9] = (s.st_gid != getegid ()) ? Qt : Qnil;
-#endif	/* not BSD4_2 */
+  values[9] = Qt;
   values[10] = INTEGER_TO_CONS (s.st_ino);
   values[11] = INTEGER_TO_CONS (s.st_dev);
 

=== modified file 'src/editfns.c'
--- src/editfns.c	2012-12-08 17:19:51 +0000
+++ src/editfns.c	2012-12-09 00:50:02 +0000
@@ -1272,6 +1272,24 @@
   return make_fixnum_or_float (uid);
 }
 
+DEFUN ("group-gid", Fgroup_gid, Sgroup_gid, 0, 0, 0,
+       doc: /* Return the effective gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t egid = getegid ();
+  return make_fixnum_or_float (egid);
+}
+
+DEFUN ("group-real-gid", Fgroup_real_gid, Sgroup_real_gid, 0, 0, 0,
+       doc: /* Return the real gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t gid = getgid ();
+  return make_fixnum_or_float (gid);
+}
+
 DEFUN ("user-full-name", Fuser_full_name, Suser_full_name, 0, 1, 0,
        doc: /* Return the full name of the user logged in, as a string.
 If the full name corresponding to Emacs's userid is not known,
@@ -4899,6 +4917,8 @@
   defsubr (&Suser_real_login_name);
   defsubr (&Suser_uid);
   defsubr (&Suser_real_uid);
+  defsubr (&Sgroup_gid);
+  defsubr (&Sgroup_real_gid);
   defsubr (&Suser_full_name);
   defsubr (&Semacs_pid);
   defsubr (&Scurrent_time);

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

No further comment, so I installed the patch as trunk bzr 111233
and am marking the bug as done.

bug#13125: Fix permissions bugs with setgid directories etc.

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I'm attaching a patch, which changes file-attributes so that it now
> outputs a placeholder value instead of the old 9th attribute member,
> since the value is rarely needed and almost nobody seems to be using
> it or caring that it's wrong.  Instead, the patch moves this
> functionality to file-ownership-preserved-p via a new argument GROUP.
> The patch also adds new functions group-gid and group-real-gid for use
> with the backup-file heuristic.
>
> This patch is relative to trunk bzr 111160.

You have also patched tramp-sh.el, thanks. However, Tramp has its own
life outside Emacs, the change must be backwards compatible for older
Emacs versions as well as for XEmacs.

Best regards, Michael.

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

On 12/09/2012 12:32 AM, Michael Albinus wrote:
> the change must be backwards compatible for older
> Emacs versions as well as for XEmacs

Thanks for mentioning that.
The following further patch should suffice:

=== modified file 'lisp/net/tramp-sh.el'
--- lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
+++ lisp/net/tramp-sh.el	2012-12-09 08:53:55 +0000
@@ -5024,8 +5024,8 @@
   (if (equal id-format 'integer) (user-uid) (user-login-name)))
 
 (defun tramp-get-local-gid (id-format)
-  (if (equal id-format 'integer)
-      (group-gid)
+  (if (and (fboundp 'group-gid) (equal id-format 'integer))
+      (tramp-compat-funcall 'group-gid)
     (nth 3 (tramp-compat-file-attributes "~/" id-format))))
 
 ;; Some predefined connection properties.

bug#13125: Fix permissions bugs with setgid directories etc.

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

> The following further patch should suffice:
>
> === modified file 'lisp/net/tramp-sh.el'
> --- lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
> +++ lisp/net/tramp-sh.el	2012-12-09 08:53:55 +0000
> @@ -5024,8 +5024,8 @@
>    (if (equal id-format 'integer) (user-uid) (user-login-name)))
>  
>  (defun tramp-get-local-gid (id-format)
> -  (if (equal id-format 'integer)
> -      (group-gid)
> +  (if (and (fboundp 'group-gid) (equal id-format 'integer))
> +      (tramp-compat-funcall 'group-gid)
>      (nth 3 (tramp-compat-file-attributes "~/" id-format))))
>  
>  ;; Some predefined connection properties.

Thanks!

Best regards, Michael.

bug#13125: Fix permissions bugs with setgid directories etc.

[Wolfgang Jenkner]

On Sun, Dec 09 2012, Paul Eggert wrote:

> In several places Emacs assumes that on 4.2BSD hosts, a newly created
> file is given a group ID equal to its parent directory, and that on
> non-4.2BSD hosts the new files are given Emacs's group ID.  Although
> this was true long ago, it hasn't been true for many years.  Most
> commonly, the old 4.2BSD behavior is now selected by the setgid bit on
> directories.

I understand you are describing here the most common behaviour only for
non-4.2BSD descendants?

I've tested your patch by typing the following in a *shell* buffer.

[[1 ~]]$ uname -rs
FreeBSD 9.1-PRERELEASE
[[2 ~]]$ id
uid=1002(wolfgang) gid=20(staff) groups=20(staff),0(wheel),5(operator)
[[3 ~]]$ ls -ld /tmp
drwxrwxrwt  8 root  wheel  512 Dec  9 16:59 /tmp/
[[4 ~]]$ rm -f /tmp/foo && touch $_
[[5 ~]]$ ls -l $_
-rw-r--r--  1 wolfgang  wheel  0 Dec  9 17:01 /tmp/foo
[[6 ~]]$ 

Then, in the same emacs process, I evaluate

(file-ownership-preserved-p "/tmp/foo")
=> t

which is fine, but

(file-ownership-preserved-p "/tmp/foo" t)
=> nil

is not since /tmp/foo will always be created in the wheel group.
Indeed, in an unpatched emacs, I get the expected

(nth 9 (file-attributes "/tmp/foo"))
=> nil

Now, open(2) on all free BSD descendants invariably, literally and
unconditionally states

     When a new file is created it is given the group of the directory which
     contains it.

So I wonder if the following lightly tested patch (on top of yours)
would give better results in this case (in the absence of races with
other processes).

Wolfgang

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-09 15:29:12 +0000
+++ lisp/files.el	2012-12-09 16:25:09 +0000
@@ -4039,6 +4039,7 @@
 		     (and (eq system-type 'windows-nt)
 			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
 		 (or (not group)
+		     (memq system-type '(berkeley-unix darwin))
 		     (= (nth 3 attributes) (group-gid)))
 		 (let* ((parent (or (file-name-directory file) "."))
 			(parent-attributes (file-attributes parent 'integer)))
@@ -4052,7 +4053,10 @@
 			;; inherits that directory's group.  On some systems
 			;; this happens even if the setgid bit is not set.
 			(or (not group)
-			    (= (nth 3 parent-attributes) (group-gid)))))))))))
+			    (= (nth 3 parent-attributes)
+			       (if (memq system-type '(berkeley-unix darwin))
+				   (nth 3 attributes)
+				 (group-gid))))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

bug#13125: Fix permissions bugs with setgid directories etc.

[Paul Eggert]

On 12/09/2012 08:43 AM, Wolfgang Jenkner wrote:

> I understand you are describing here the most common behaviour only for
> non-4.2BSD descendants?

Yes, that's right.

> (in the absence of races with other processes)

Yes, races are a problem, both with current Emacs and with the patch.
It'd be good to fix this separate problem, when someone finds the time.
At least the proposed patch does not make things worse in this respect.

> Now, open(2) on all free BSD descendants invariably, literally and
> unconditionally states
> 
>      When a new file is created it is given the group of the directory which
>      contains it.

I was worried about what happens with a BSD client of an
NFS server running some non-BSD OS.  But if it's safe to
assume BSD semantics even then, your suggestion is a good one,
as it'll make Emacs more efficient.

Two thoughts.  First, shouldn't gnu/kfreebsd be treated as a BSD
system in this respect?  Second, the second part of the test can
be simplified a tad.  So, how about the following patch instead?

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-09 00:50:02 +0000
+++ lisp/files.el	2012-12-10 00:38:45 +0000
@@ -4039,6 +4039,9 @@ the group would be preserved too."
 		     (and (eq system-type 'windows-nt)
 			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
 		 (or (not group)
+		     ;; On BSD-derived systems files always inherit the parent
+		     ;; directory's group, so skip the group-gid test.
+		     (memq system-type '(berkeley-unix darwin gnu/kfreebsd))
 		     (= (nth 3 attributes) (group-gid)))
 		 (let* ((parent (or (file-name-directory file) "."))
 			(parent-attributes (file-attributes parent 'integer)))
@@ -4052,7 +4055,8 @@ the group would be preserved too."
 			;; inherits that directory's group.  On some systems
 			;; this happens even if the setgid bit is not set.
 			(or (not group)
-			    (= (nth 3 parent-attributes) (group-gid)))))))))))
+			    (= (nth 3 parent-attributes)
+			       (nth 3 attributes)))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".