ATTRIB.EXE +r doesn't work when inheriting permissions

[Buck Huppmann]

I'm still unclear on how ``inherit permissions'' is supposed to work, but
i'd think that it wouldn't contravene an explicit user request to make a
file read-only (or change the permissions on an existing file just b/c a
user writes to it, as previously lamented in
http://lists.samba.org/pipermail/samba-technical/2001-December/033396.html
). This would seemingly be ``force [directory] security mode'''s job

So i can't make a file read-only via the NT explorer ``General'' property
sheet check box or the ATTRIB.EXE command, although using the ACL-tweak-
ing interface allows me to override inherited writability just fine. The
problem seems to be that smbd/dosmode.c:file_chmod() calls :unix_mode()
with an IS_DOS_READONLY(dosmode) but gets back a mode that's writable if
the parent's mode is. I guess the easiest ``fix'', if you consider this a
bug, is as follows

--- patch ---

--- samba-2.2.2/source/smbd/dosmode.c.orig	Fri Apr 13 19:24:46 2001
+++ samba-2.2.2/source/smbd/dosmode.c	Fri Feb  1 15:08:18 2002
@@ -47,12 +47,9 @@
 ****************************************************************************/
 mode_t unix_mode(connection_struct *conn,int dosmode,const char *fname)
 {
-  mode_t result = (S_IRUSR | S_IRGRP | S_IROTH);
+  mode_t result;
   mode_t dir_mode = 0; /* Mode of the parent directory if inheriting. */
 
-  if ( !IS_DOS_READONLY(dosmode) )
-    result |= (S_IWUSR | S_IWGRP | S_IWOTH);
-
   if (fname && lp_inherit_perms(SNUM(conn))) {
     char *dname;
     SMB_STRUCT_STAT sbuf;
@@ -67,9 +64,22 @@
     /* Save for later - but explicitly remove setuid bit for safety. */
     dir_mode = sbuf.st_mode & ~S_ISUID;
     DEBUG(2,("unix_mode(%s) inherit mode %o\n",fname,(int)dir_mode));
-    /* Clear "result" */
-    result = 0;
+    /* initialize result with parent mode as template */
+    result = dir_mode;
+    /* for files, the only bits we inherit are the rw's */
+    if (!IS_DOS_DIR(dosmode))
+      result &= S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH;
   } 
+  else {
+    /* initialize result with rw-rw-rw- as template. all -x bits are
+     * picked up below, both for directories and for file x-bit hacks */
+    result = S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH;
+  }
+
+  /* if the user asks for read-only, give her/him read-only (subject to
+   * any force_..._mode()'s applied later) */
+  if (IS_DOS_READONLY(dosmode))
+    result &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
 
   if (IS_DOS_DIR(dosmode)) {
     /* We never make directories read only for the owner as under DOS a user
@@ -77,8 +87,7 @@
     result |= (S_IFDIR | S_IWUSR);
 
     if (dir_mode) {
-      /* Inherit mode of parent directory. */
-      result |= dir_mode;
+	;
     } else {
       /* Provisionally add all 'x' bits */
       result |= (S_IXUSR | S_IXGRP | S_IXOTH);                 
@@ -99,9 +108,7 @@
       result |= S_IXOTH;  
 
     if (dir_mode) {
-      /* Inherit 666 component of parent directory mode */
-      result |= dir_mode
-        &  (S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH);
+	;
     } else {
       /* Apply mode mask */
       result &= lp_create_mask(SNUM(conn));
@@ -228,9 +235,17 @@
 		unixmode |= tmp;
 	}
 
-	/* if we previously had any w bits set then leave them alone 
-		whilst adding in the new w bits, if the new mode is not rdonly */
+	/* if the new mode is not aRONLY, reset write bits (presumably
+	 * including owner-write, but maybe not) per the config IFF the
+	 * current UNIX mode doesn't already grant owner write access.
+	 * this is the conservative heuristic for not resetting write bits
+	 * on files if we're called and the desired change is only from
+	 * non-archive to archive, say, and not a change in writability,
+	 * as it accords with dos_mode()'s mapping of S_IWUSR to DOS
+	 * ~aRONLY, at present */
 	if (!IS_DOS_READONLY(dosmode)) {
+		if (st->st_mode & S_IWUSR)
+			unixmode &= ~(S_IWUSR|S_IWGRP|S_IWOTH);
 		unixmode |= (st->st_mode & (S_IWUSR|S_IWGRP|S_IWOTH));
 	}

--- end of patch ---

The last hunk is my previously proffered patch from the above-referenced
message. More prudent might be to separate out the initial-permission-set-
ting aspect of file_chmod() from the, well, changing aspect, somewhat as
follows, but you're probably not going to do this

At any rate, best of luck to 2.2.3, and keep up the good work

--buck

/* chmod() for the times you want to set the mode */
int file_initmod(...)
{
	/* old file_chmod() */
}

/* chmod() for the times you only want to change some mappings */
int file_altermod(...)
{
	mode_t mask = 0, unixmode;
	int dosmode_diff;
	if (!st) { ... }
	dosmode_diff = dosmode ^ dos_mode(...);
	if (!dosmode_diff)
		return 0;
	if (IS_DOS_DIR(dosmode_diff))
		drop_dead_laughing();
	/* we don't touch any r bits, since that doesn't correspond
	 * to any change of a DOS attribute */
	if (IS_DOS_READONLY(dosmode_diff))
		mask |= S_IWUSR | S_IWGRP | S_IWOTH;
	if (MAP_ARCHIVE(conn) && IS_DOS_ARCHIVE(dosmode_diff))
		mask |= S_IXUSR;
	if (MAP_SYSTEM(conn) && IS_DOS_SYSTEM(dosmode_diff))
		mask |= S_IXGRP;
	if (MAP_SYSTEM(conn) && IS_DOS_HIDDEN(dosmode_diff))
		mask |= S_IXOTH;
	/* any more ? */
	unixmode = st->st_mode & ~mask | unix_mode(...) & mask;

	/* actual chmod(), perhaps calling into a common sub factored out
	   from the original file_chmod() */
}