[PATCH e2fsprogs] subst: use 0644 perms

[PATCH e2fsprogs] subst: use 0644 perms

[Mike Frysinger]

When running on NFS, opening files with 0444 perms for writing can
sometimes fail.  Since there's no real reason for these files to be
read-only, give the owner write permission.

URL: https://bugs.gentoo.org/550986
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 util/subst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/subst.c b/util/subst.c
index f36adb4..e4004c9 100644
--- a/util/subst.c
+++ b/util/subst.c
@@ -370,7 +370,7 @@ int main(int argc, char **argv)
 		}
 		strcpy(newfn, outfn);
 		strcat(newfn, ".new");
-		fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0444);
+		fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0644);
 		if (fd < 0) {
 			perror(newfn);
 			exit(1);
-- 
2.5.1

Re: [PATCH e2fsprogs] subst: use 0644 perms

[Andreas Dilger]

On Sep 18, 2015, at 01:54, Mike Frysinger <vapier@gentoo.org> wrote:
> 
> When running on NFS, opening files with 0444 perms for writing can
> sometimes fail.  Since there's no real reason for these files to be
> read-only, give the owner write permission.

Actually, there IS a reason for subst to make these files read-only. They are auto-generated and any edits to these files can be overwritten and lost if their origin files are modified. 

I'd lost edits to these auto generated files many time because they are the ones that "tags" or "cscope" will jump to when searching for symbols. 

There really isn't any reason for them to be writable, so the fact that you are getting an error trying to open them for writing is a hint that you are doing, or going to do, the wrong thing and the read-only nature of the file is preventing you from going down the wrong path. 

Cheers, Andreas

> URL: https://bugs.gentoo.org/550986
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> util/subst.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/subst.c b/util/subst.c
> index f36adb4..e4004c9 100644
> --- a/util/subst.c
> +++ b/util/subst.c
> @@ -370,7 +370,7 @@ int main(int argc, char **argv)
>        }
>        strcpy(newfn, outfn);
>        strcat(newfn, ".new");
> -        fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0444);
> +        fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0644);
>        if (fd < 0) {
>            perror(newfn);
>            exit(1);
> -- 
> 2.5.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH e2fsprogs] subst: use 0644 perms

[Mike Frysinger]

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

On 18 Sep 2015 10:52, Andreas Dilger wrote:
> On Sep 18, 2015, at 01:54, Mike Frysinger <vapier@gentoo.org> wrote:
> > When running on NFS, opening files with 0444 perms for writing can
> > sometimes fail.  Since there's no real reason for these files to be
> > read-only, give the owner write permission.
> 
> Actually, there IS a reason for subst to make these files read-only. They are auto-generated and any edits to these files can be overwritten and lost if their origin files are modified. 
> 
> I'd lost edits to these auto generated files many time because they are the ones that "tags" or "cscope" will jump to when searching for symbols. 
> 
> There really isn't any reason for them to be writable, so the fact that you are getting an error trying to open them for writing is a hint that you are doing, or going to do, the wrong thing and the read-only nature of the file is preventing you from going down the wrong path. 

i think you misread my report.  this has nothing to do with people trying
to modify the files after the fact.  NFS can (and sometimes does) throw an
error at the time of the *open* call even if the file doesn't exist.

if you want to try to "protect" people, then it needs to be a chmod after
all the data has been written & closed.  this is how it used to behave,
but commit 2873927d15ffb9ee9ed0e2700791a0e519c715aa changed it.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH e2fsprogs] subst: use 0644 perms

[Andreas Dilger]

On Sep 18, 2015, at 12:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> 
> On 18 Sep 2015 10:52, Andreas Dilger wrote:
>> On Sep 18, 2015, at 01:54, Mike Frysinger <vapier@gentoo.org> wrote:
>>> When running on NFS, opening files with 0444 perms for writing can
>>> sometimes fail.  Since there's no real reason for these files to be
>>> read-only, give the owner write permission.
>> 
>> Actually, there IS a reason for subst to make these files read-only. They are auto-generated and any edits to these files can be overwritten and lost if their origin files are modified. 
>> 
>> I'd lost edits to these auto generated files many time because they are the ones that "tags" or "cscope" will jump to when searching for symbols. 
>> 
>> There really isn't any reason for them to be writable, so the fact that you are getting an error trying to open them for writing is a hint that you are doing, or going to do, the wrong thing and the read-only nature of the file is preventing you from going down the wrong path. 
> 
> I think you misread my report.  this has nothing to do with people
> trying to modify the files after the fact.  NFS can (and sometimes
> does) throw an error at the time of the *open* call even if the file
> doesn't exist.

Seems like a bug in NFS.  At least I know the POSIX test suite requires
that to work (we had similar problems with Lustre that had to be fixed many years ago).  In fact, there is a special check for this in knfsd:


nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
                                        struct dentry *dentry, int acc)
{
    :
    :

    /*
     * The file owner always gets access permission for accesses that
     * would normally be checked at open time. This is to make
     * file access work even when the client has done a fchmod(fd, 0).
     *
     * However, `cp foo bar' should fail nevertheless when bar is
     * readonly. A sensible way to do this might be to reject all
     * attempts to truncate a read-only file, because a creat() call
     * always implies file truncation.
     * ... but this isn't really fair.  A process may reasonably call
     * ftruncate on an open file descriptor on a file with perm 000.
     * We must trust the client to do permission checking - using "ACCESS"
     * with NFSv3.
     */
    if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
        uid_eq(inode->i_uid, current_fsuid()))
            return 0;
    :
    :
}

Might be something that needs to be pursued with the kernel NFS folks?
That said, I'm not against fixing subst so that it works on your system.

> if you want to try to "protect" people, then it needs to be a chmod
> after all the data has been written & closed.

It that case, please update your patch to include something like:

                        if (verbose)
                                printf("Creating or replacing %s.\n", outfn);
+                       /* Avoid accidentally editing generated file. */
+                       (void)fchmod(out, 0444);
                        fclose(out);
                        if (old)
                                fclose(old);

>  this is how it used to
> behave, but commit 2873927d15ffb9ee9ed0e2700791a0e519c715aa changed it.


Cheers, Andreas

Re: [PATCH e2fsprogs] subst: use 0644 perms

[Theodore Ts'o]

On Fri, Sep 18, 2015 at 02:08:24PM -0400, Mike Frysinger wrote:
> 
> i think you misread my report.  this has nothing to do with people trying
> to modify the files after the fact.  NFS can (and sometimes does) throw an
> error at the time of the *open* call even if the file doesn't exist.

I believe Andreas did understand your report; he was just objecting to
the claim in the git description that there is "no reason" to have the
files generated subst to be read-only.

> if you want to try to "protect" people, then it needs to be a chmod after
> all the data has been written & closed.  this is how it used to behave,
> but commit 2873927d15ffb9ee9ed0e2700791a0e519c715aa changed it.

I think Andreas was asking you to make this change to the patch.  I
had a bit of spare time (thanks to perfcrastination :-), so I took
care of it.

      	   	      		       	       - Ted

commit e5a82003d1b3b7ea01f60dadb49c3bbc60e4ebb7
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Fri Sep 18 21:37:53 2015 -0400

    subst: work around an NFS bug
    
    When running on NFS, opening files with 0444 perms for writing can
    sometimes fail.  This is arguably an NFS server bug, but work around
    it by creating the file with 0644 permissions, and only change the
    permissions to be 0444 right before we close the file.
    
    URL: https://bugs.gentoo.org/550986
    Reported-by: Mike Frysinger <vapier@gentoo.org>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/util/subst.c b/util/subst.c
index f36adb4..70dc0bc 100644
--- a/util/subst.c
+++ b/util/subst.c
@@ -319,7 +319,7 @@ int main(int argc, char **argv)
 {
 	char	line[2048];
 	int	c;
-	int	fd;
+	int	fd, ofd = -1;
 	FILE	*in, *out, *old = NULL;
 	char	*outfn = NULL, *newfn = NULL;
 	int	verbose = 0;
@@ -370,12 +370,12 @@ int main(int argc, char **argv)
 		}
 		strcpy(newfn, outfn);
 		strcat(newfn, ".new");
-		fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0444);
-		if (fd < 0) {
+		ofd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0644);
+		if (ofd < 0) {
 			perror(newfn);
 			exit(1);
 		}
-		out = fdopen(fd, "w+");
+		out = fdopen(ofd, "w+");
 		if (!out) {
 			perror("fdopen");
 			exit(1);
@@ -429,12 +429,16 @@ int main(int argc, char **argv)
 					printf("Using original atime\n");
 				set_utimes(outfn, fileno(old), tv);
 			}
+			if (ofd >= 0)
+				(void) fchmod(ofd, 0444);
 			fclose(out);
 			if (unlink(newfn) < 0)
 				perror("unlink");
 		} else {
 			if (verbose)
 				printf("Creating or replacing %s.\n", outfn);
+			if (ofd >= 0)
+				(void) fchmod(ofd, 0444);
 			fclose(out);
 			if (old)
 				fclose(old);

[PATCH v2 e2fsprogs] subst: use 0644 perms

[Mike Frysinger]

When running on NFS, opening files with 0444 perms for writing can
sometimes fail.  Delay the permission update until the end (which
restores behavior to pre-commit 2873927d15ffb9ee9ed0e2700791a0e5).

URL: https://bugs.gentoo.org/550986
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 util/subst.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/subst.c b/util/subst.c
index f36adb4..10ad6b9 100644
--- a/util/subst.c
+++ b/util/subst.c
@@ -370,7 +370,7 @@ int main(int argc, char **argv)
 		}
 		strcpy(newfn, outfn);
 		strcat(newfn, ".new");
-		fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0444);
+		fd = open(newfn, O_CREAT|O_TRUNC|O_RDWR, 0644);
 		if (fd < 0) {
 			perror(newfn);
 			exit(1);
@@ -443,6 +443,11 @@ int main(int argc, char **argv)
 				perror("rename");
 				exit(1);
 			}
+			/* set read-only to alert user it is a generated file */
+			if (chmod(outfn, 0444)) {
+				perror("chmod");
+				exit(1);
+			}
 		}
 	}
 	if (old)
-- 
2.5.2

Re: [PATCH v2 e2fsprogs] subst: use 0644 perms

[Theodore Ts'o]

On Mon, Sep 28, 2015 at 09:14:38PM -0400, Mike Frysinger wrote:
> When running on NFS, opening files with 0444 perms for writing can
> sometimes fail.  Delay the permission update until the end (which
> restores behavior to pre-commit 2873927d15ffb9ee9ed0e2700791a0e5).
> 
> URL: https://bugs.gentoo.org/550986
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

I've already applied my version of the patch which uses fchmod instead
of chmod (to avoid static Code Analysis systems from complaining).

Cheers,

   	     	   	       			- Ted