NetBSD Problem Report #56775

From www@netbsd.org  Thu Mar 31 19:06:29 2022
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 13EC81A921F
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 31 Mar 2022 19:06:29 +0000 (UTC)
Message-Id: <20220331190627.90C811A9239@mollari.NetBSD.org>
Date: Thu, 31 Mar 2022 19:06:27 +0000 (UTC)
From: rvp@SDF.ORG
Reply-To: rvp@SDF.ORG
To: gnats-bugs@NetBSD.org
Subject: sort -o foo foo can sometines remove foo
X-Send-Pr-Version: www-1.0

>Number:         56775
>Category:       bin
>Synopsis:       sort -o foo foo can sometines remove foo
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 31 19:10:00 +0000 2022
>Last-Modified:  Mon Apr 04 21:40:01 +0000 2022
>Originator:     RVP
>Release:        NetBSD/amd64 9.99.96
>Organization:
>Environment:
NetBSD x202e.localdomain 9.99.96 NetBSD 9.99.96 (MYKERNEL) #0: Thu Mar 31 09:03:09 UTC 2022  bld@x202e.localdomain:/usr/obj/usr/src/sys/arch/amd64/compile/MYKERNEL amd64
>Description:
Under certain circumstances, `sort -o foo foo' can end up deleting `foo':

$ sudo sysctl -w security.models.extensions.hardlink_check_uid=1
$ sudo sysctl -w security.models.extensions.hardlink_check_gid=1
$ cd /tmp               # cd to a world-writable dir.
$ >foo
$ ls -l foo             # note that group is `wheel' as per BSD
                        # semantics for /tmp
-rw-------  1 rvp  wheel  0 Mar 31 08:32 foo
$ sort -o foo foo
sort: cannot link foo: output left in foopSQspp: Permission denied
$ ls -l foo
ls: foo: No such file or directory
$ 

Input file is destroyed if user doesn't belong to group `wheel'--even
though files can be created and renamed by anyone in /tmp.

This results in this error message (caused by running lorder) while
building the kernel:

[...]
    compile  kern/umodti3.o
    compile  kern/xlat_mbr_fstype.o
building standard kern library
sort: cannot link /tmp/_reference_.psA3FN: output left in /tmp/_reference_.psA3FNlDcAku: Permission denied
sort: cannot link /tmp/_symbol_.Gy4pSK: output left in /tmp/_symbol_.Gy4pSKrHWftZ: Permission denied
nbjoin: /tmp/_reference_.psA3FN: No such file or directory
     create  vers.c
[...]

>How-To-Repeat:
As above.
>Fix:
Use rename(2):

diff -urN usr.bin/sort.orig/sort.c usr.bin/sort/sort.c
--- usr.bin/sort.orig/sort.c	2017-01-10 21:13:45.000000000 +0000
+++ usr.bin/sort/sort.c	2022-03-31 09:17:25.991550462 +0000
@@ -329,7 +329,7 @@
 		errno = 0;
 		if (access(outpath, W_OK))
 			err(2, "%s", outpath);
-		(void)snprintf(toutpath, sizeof(toutpath), "%sXXXXXX",
+		(void)snprintf(toutpath, sizeof(toutpath), "%s.XXXXXX",
 		    outpath);
 		if ((outfd = mkstemp(toutpath)) == -1)
 			err(2, "Cannot create temporary file `%s'", toutpath);
@@ -368,11 +368,8 @@
 			    outpath, outfile);
 		}

-		(void)unlink(outpath);
-		if (link(outfile, outpath))
-			err(2, "cannot link %s: output left in %s",
-			    outpath, outfile);
-		(void)unlink(outfile);
+		if (rename(outfile, outpath) == -1)
+			err(2, "rename failed: %s->%s", outfile, outpath);
 		toutpath[0] = 0;
 	}
 	exit(0);

>Audit-Trail:
From: Thomas Klausner <wiz@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
Date: Thu, 31 Mar 2022 21:19:06 +0200

 On Thu, Mar 31, 2022 at 07:10:00PM +0000, rvp@SDF.ORG wrote:
 > @@ -368,11 +368,8 @@
 >  			    outpath, outfile);
 >  		}
 >  
 > -		(void)unlink(outpath);
 > -		if (link(outfile, outpath))
 > -			err(2, "cannot link %s: output left in %s",
 > -			    outpath, outfile);
 > -		(void)unlink(outfile);
 > +		if (rename(outfile, outpath) == -1)
 > +			err(2, "rename failed: %s->%s", outfile, outpath);
 >  		toutpath[0] = 0;
 >  	}
 >  	exit(0);
 > 

 Shouldn't the there be an

 	  unlink(outfile);

 in the error case?
  Thomas

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
Date: Thu, 31 Mar 2022 21:31:11 +0000 (UTC)

 On Thu, 31 Mar 2022, Thomas Klausner wrote:

 > Shouldn't the there be an
 >
 > 	  unlink(outfile);
 >
 > in the error case?
 >

 rename(2) will do that.

 -RVP

From: Thomas Klausner <wiz@NetBSD.org>
To: NetBSD bugtracking <gnats-bugs@NetBSD.org>
Cc: 
Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
Date: Thu, 31 Mar 2022 23:49:16 +0200

 On Thu, Mar 31, 2022 at 09:35:01PM +0000, RVP wrote:
 > The following reply was made to PR bin/56775; it has been noted by GNATS.
 > 
 > From: RVP <rvp@SDF.ORG>
 > To: gnats-bugs@netbsd.org
 > Cc: 
 > Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
 > Date: Thu, 31 Mar 2022 21:31:11 +0000 (UTC)
 > 
 >  On Thu, 31 Mar 2022, Thomas Klausner wrote:
 >  
 >  > Shouldn't the there be an
 >  >
 >  > 	  unlink(outfile);
 >  >
 >  > in the error case?
 >  >
 >  
 >  rename(2) will do that.

 If the rename fails, it won't. That's why I wrote "in the error case".
  Thomas

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
Date: Thu, 31 Mar 2022 21:59:24 +0000 (UTC)

 On Thu, 31 Mar 2022, Thomas Klausner wrote:

 > If the rename fails, it won't. That's why I wrote "in the error case".
 >  Thomas
 >

 cleanup() will remove the temp. file.

 -RVP

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56775: sort -o foo foo can sometines remove foo
Date: Mon, 4 Apr 2022 21:39:51 +0000 (UTC)

 Noticed another problem with sort: the -o flag doesn't preserve
 the original file type. FIFOs and symlinks are turned into ordinary
 files:

 $ >foo
 $ ln -s foo bar
 $ ls -l bar
 lrwx------  1 rvp  wheel  3 Apr  4 21:04 bar -> foo
 $ sort -o bar bar
 $ ls -l bar
 -rwx------  1 rvp  wheel  0 Apr  4 21:05 bar
 $

 The straight-forward "fix" of changing the test on line 323 to:

  	&& S_ISREG(st.st_mode)) {

 will deadlock on FIFOs because both the input and output files will
 be the same (and have to be kept open). I'm copying the temp. file
 into the FIFO in this case. (It also doesn't work if the files are
 symlinks--and they, in turn, point to other symlinks. I'm also
 assuming that following symlinks is the correct behaviour.)

 The patch below also includes the previous patch:

 ---START---
 diff -urN usr.bin/sort.orig/sort.c usr.bin/sort/sort.c
 --- usr.bin/sort.orig/sort.c	2017-01-10 21:13:45.000000000 +0000
 +++ usr.bin/sort/sort.c	2022-04-04 20:47:12.959039129 +0000
 @@ -80,6 +80,7 @@
   #include <locale.h>
   #include <paths.h>
   #include <signal.h>
 +#include <stdbool.h>
   #include <stdlib.h>
   #include <string.h>
   #include <unistd.h>
 @@ -105,12 +106,14 @@

   unsigned int debug_flags = 0;

 -static char toutpath[MAXPATHLEN];
 +static char toutpath[MAXPATHLEN + 8];
 +static char truepath[MAXPATHLEN];

   const char *tmpdir;	/* where temporary files should be put */

   static void cleanup(void);
   static void onsignal(int);
 +static bool doesexist(char *outpath, struct stat *st);
   __dead static void usage(const char *);

   int
 @@ -319,7 +322,7 @@
   		toutpath[0] = '\0';	/* path not used in this case */
   		outfile = outpath = toutpath;
   		outfp = stdout;
 -	} else if (lstat(outpath, &st) == 0
 +	} else if (doesexist(outpath, &st)
   	    && !S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) {
   		/* output file exists and isn't character or block device */
   		struct sigaction act;
 @@ -327,9 +330,10 @@
   		    SIGXCPU, SIGXFSZ, SIGVTALRM, SIGPROF, 0};
   		int outfd;
   		errno = 0;
 +		outpath = truepath;
   		if (access(outpath, W_OK))
   			err(2, "%s", outpath);
 -		(void)snprintf(toutpath, sizeof(toutpath), "%sXXXXXX",
 +		(void)snprintf(toutpath, sizeof(toutpath), "%s.XXXXXX",
   		    outpath);
   		if ((outfd = mkstemp(toutpath)) == -1)
   			err(2, "Cannot create temporary file `%s'", toutpath);
 @@ -363,21 +367,52 @@
   		 * st is initialized above, when we create the
   		 * temporary spool file.
   		 */
 -		if (lchmod(outfile, st.st_mode & ALLPERMS) != 0) {
 +		if (chmod(outfile, st.st_mode & ALLPERMS) != 0) {
   			err(2, "cannot chmod %s: output left in %s",
   			    outpath, outfile);
   		}

 -		(void)unlink(outpath);
 -		if (link(outfile, outpath))
 -			err(2, "cannot link %s: output left in %s",
 -			    outpath, outfile);
 -		(void)unlink(outfile);
 -		toutpath[0] = 0;
 +		if (S_ISREG(st.st_mode)) {
 +			if (rename(outfile, outpath) == -1)
 +				err(2, "rename failed: %s->%s", outfile, outpath);
 +			toutpath[0] = 0;
 +		} else {	/* FIFO: copy sorted */
 +			char buf[BUFSIZ];
 +			FILE *ifp, *ofp;
 +			size_t n;
 +
 +			if (fclose(outfp))
 +				err(2, "IO error");
 +			if ((ifp = fopen(outfile, "r")) == NULL)
 +				err(2, "cannot read %s", outfile);
 +			if ((ofp = fopen(outpath, "w")) == NULL)
 +				err(2, "cannot write %s", outpath);
 +			while ((n = fread(buf, 1, sizeof buf, ifp)) > 0)
 +				if (fwrite(buf, 1, n, ofp) != n)
 +					break;
 +			if (fclose(ofp) || ferror(ifp))
 +				err(2, "IO error");
 +		}
   	}
   	exit(0);
   }

 +static bool
 +doesexist(char *outpath, struct stat *st)
 +{
 +	if (lstat(outpath, st) == -1)
 +		return false;		/* assume no file */
 +	if (!S_ISLNK(st->st_mode)) {
 +		strlcpy(truepath, outpath, sizeof truepath);
 +		return true;		/* skip a func. call */
 +	}
 +	if (realpath(outpath, truepath) == NULL)
 +		err(2, "%s: realpath failed", outpath);	/* hopeless */
 +	if (stat(truepath, st) == -1)
 +		err(2, "%s: stat failed", outpath);	/*    "     */
 +	return true;
 +}
 +
   static void
   onsignal(int sig)
   {
 ---END---

 Thanks,
 -RVP

Home	PR Database Search