Re: bin/56775: sort -o foo foo can sometines remove foo

[RVP <rvp%SDF.ORG@localhost>]

The following reply was made to PR bin/56775; it has been noted by GNATS.

From: RVP <rvp%SDF.ORG@localhost>
To: gnats-bugs%netbsd.org@localhost
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