sed(1) make -i behave a little nicer

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

sed(1) make -i behave a little nicer

Martijn van Duren-5
I ran into a few minor nuisances with sed's -i mode, which are mostly
compatible with gnu sed, but I reckon we should address.

The problem is sed works by writing the output to a second file in the
same directory as the original and after completion renaming the file
to the original. This has two disadvantages:
1) The inode number changes, resulting in loss of carefully crafted
   hardlinks.
2) We require to have write permission in the same directory as the
   original file, even if we don't want to have a backup file.

Diff below tries to solve this by doing the following.
Copy the file to the backup location (/tmp/sedXXXXXXXXXX if no
extension) and use the backup as the infile and the original as the
outfile.

Furthermore I changed the lstat to fstat, so we can edit symlinks (gsed
supports symlinks by replacing the symlink by a new real file, which is
also fun), and I extended the warning messages in process to show the
backup file if we crash during operation, so people will always know
where to recover the file in case of disaster.

Because process also error(FATAL, ...)s during process and we always
have a backup file I don't think the warning in sed.1 is worth keeping.

The only downside to this new approach (that I can think of) is that we
now temporarily have a file that is in an inconsistent state, but that's
not much different to writing a file with any other editor.

$ echo test > /usr/obj/test && dd if=/dev/zero of=/usr/obj/bloat
/usr/obj: write failed, file system is full
dd: /usr/obj/bloat: No space left on device
614113+0 records in
614112+0 records out
314425344 bytes transferred in 2.206 secs (142470196 bytes/sec)
$ ./obj/sed -i -f /tmp/test.sed /usr/obj/test                                                                      

/usr/obj: write failed, file system is full
sed: /usr/obj/test: No space left on device (backup at /tmp/sedgRPSLImG9N)
$ cat /tmp/test.sed
s/test/aaa<repeat to infinity>/
$ cat /tmp/sedgRPSLImG9N
test
$ ls -i /tmp/test
104 /tmp/test
$ sed -i s/test/foo/ /tmp/test
$ ls -i /tmp/test            
104 /tmp/test
$ doas touch /etc/test
$ doas chown martijn /etc/test
$ echo test > /etc/test
$ sed -i s/test/foo/ /etc/test
$ cat /etc/test
foo

The diff does change quite a few mechanics, so some scrutiny is welcome.

martijn@

Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.39
diff -u -p -r1.39 main.c
--- main.c 6 Dec 2018 20:16:04 -0000 1.39
+++ main.c 7 Dec 2018 07:31:54 -0000
@@ -35,6 +35,7 @@
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
+#include <sys/param.h>
 #include <sys/stat.h>
 
 #include <ctype.h>
@@ -94,13 +95,13 @@ static int rval; /* Exit status */
  */
 const char *fname; /* File name. */
 const char *outfname; /* Output file name */
-static char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
-static char tmpfname[PATH_MAX]; /* Temporary file name (for in-place editing) */
+char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
 char *inplace; /* Inplace edit file extension */
 u_long linenum;
 
 static void add_compunit(enum e_cut, char *);
 static void add_file(char *);
+int copy(FILE *, FILE *);
 static int next_files_have_lines(void);
 
 int termwidth;
@@ -310,26 +311,46 @@ again:
  return (NULL);
 }
 
+int
+copy(FILE *src, FILE *dst)
+{
+ unsigned char buf[MAXBSIZE];
+ size_t r, w, tw;
+
+ while(1) {
+ if ((r = fread(buf, sizeof(*buf), sizeof(buf), src)) == 0) {
+ if (feof(src)) {
+ if (fflush(dst) == EOF)
+ return 0;
+ return 1;
+ }
+ if (errno != EINTR)
+ return 0;
+ continue;
+ }
+ tw = 0;
+ while(tw < r) {
+ w = fwrite(buf + tw, sizeof(*buf), r - tw, dst);
+ if (w == 0) {
+ if (errno != EINTR)
+ return 0;
+ continue;
+ }
+ tw += w;
+ }
+ }
+}
+
 void
 finish_file(void)
 {
  if (infile != NULL) {
  fclose(infile);
- if (*oldfname != '\0') {
- if (rename(fname, oldfname) != 0) {
- warning("rename()");
- unlink(tmpfname);
- exit(1);
- }
+ if (inplace != NULL) {
+ if (*oldfname == '\0')
+ unlink(oldfname);
  *oldfname = '\0';
  }
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
  outfname = NULL;
  }
 }
@@ -341,7 +362,7 @@ finish_file(void)
 int
 mf_fgets(SPACE *sp, enum e_spflag spflag)
 {
- struct stat sb;
+ struct stat sb, isb;
  size_t len;
  char *p;
  int c, fd;
@@ -369,10 +390,18 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  outfname = "stdout";
  break;
  }
+ infile = fopen(fname, inplace == NULL ? "r" : "r+");
+ if (infile == NULL) {
+ warning("%s", strerror(errno));
+ rval = 1;
+ continue;
+ }
  if (inplace != NULL) {
- if (lstat(fname, &sb) != 0)
+ /* inplace reads from copy */
+ outfile = infile;
+ if (fstat(fileno(outfile), &sb) != 0)
  error(FATAL, "%s: %s", fname,
-    strerror(errno ? errno : EIO));
+    strerror(errno));
  if (!S_ISREG(sb.st_mode))
  error(FATAL, "%s: %s %s", fname,
     "in-place editing only",
@@ -382,32 +411,48 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
     sizeof(oldfname));
  len = strlcat(oldfname, inplace,
     sizeof(oldfname));
- if (len > sizeof(oldfname))
- error(FATAL, "%s: name too long", fname);
- }
- len = snprintf(tmpfname, sizeof(tmpfname), "%s/sedXXXXXXXXXX",
-    dirname(fname));
- if (len >= sizeof(tmpfname))
- error(FATAL, "%s: name too long", fname);
- if ((fd = mkstemp(tmpfname)) == -1)
- error(FATAL, "%s: %s", fname, strerror(errno));
- if ((outfile = fdopen(fd, "w")) == NULL) {
- unlink(tmpfname);
- error(FATAL, "%s", fname);
+ if (len >= sizeof(oldfname))
+ error(FATAL, "backup %s: name too long",
+    fname);
+ (void)unlink(oldfname);
+ if ((infile = fopen(oldfname, "w+")) == NULL)
+ error(FATAL, "can't create backup "
+    "file: %s", oldfname);
+ if (fstat(fileno(infile), &isb) != 0)
+ error(FATAL, "%s: %s", oldfname,
+    strerror(errno));
+ if (!S_ISREG(isb.st_mode))
+ error(FATAL, "backup file %s not a "
+    "regular file and can't be "
+    "replaced", oldfname);
+ fchown(fileno(infile), sb.st_uid, sb.st_gid);
+ fchmod(fileno(infile), sb.st_mode & ALLPERMS);
+ } else {
+ (void)strlcpy(oldfname, "/tmp/sedXXXXXXXXXX",
+    sizeof(oldfname));
+ if ((fd = mkstemp(oldfname)) == -1)
+ error(FATAL, "%s: %s", fname,
+    strerror(errno));
+ if ((infile = fdopen(fd, "a+")) == NULL) {
+ unlink(oldfname);
+ error(FATAL, "%s", fname);
+ }
  }
- fchown(fileno(outfile), sb.st_uid, sb.st_gid);
- fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
- outfname = tmpfname;
+ outfname = fname;
  linenum = 0;
  resetstate();
+ if (!copy(outfile, infile)) {
+ unlink(oldfname);
+ error(FATAL, "%s: failed to create backup copy "
+    "to %s: %s", fname, oldfname,
+    strerror(errno));
+ }
+ ftruncate(fileno(outfile), 0);
+ rewind(outfile);
+ rewind(infile);
  } else {
  outfile = stdout;
  outfname = "stdout";
- }
- if ((infile = fopen(fname, "r")) == NULL) {
- warning("%s", strerror(errno));
- rval = 1;
- continue;
  }
  }
 
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.34
diff -u -p -r1.34 process.c
--- process.c 14 Nov 2018 10:59:33 -0000 1.34
+++ process.c 7 Dec 2018 07:31:54 -0000
@@ -75,6 +75,7 @@ static int sdone; /* If any substitutes
  /* Iov structure for 'w' commands. */
 static regex_t *defpreg;
 size_t maxnsub;
+extern char oldfname[PATH_MAX];
 regmatch_t *match;
 
 #define OUT() do {\
@@ -473,7 +474,11 @@ flush_appends(void)
  break;
  }
  if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+    strerror(errno ? errno : EIO),
+    *oldfname == '\0' ? "" : " (backup at ",
+    *oldfname == '\0' ? "" : oldfname,
+    *oldfname == '\0' ? "" : ")");
  appendx = sdone = 0;
 }
 
@@ -513,7 +518,11 @@ lputs(char *s)
  (void)fputc('$', outfile);
  (void)fputc('\n', outfile);
  if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+    strerror(errno ? errno : EIO),
+    *oldfname == '\0' ? "" : " (backup at ",
+    *oldfname == '\0' ? "" : oldfname,
+    *oldfname == '\0' ? "" : ")");
 }
 
 static inline int
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.57
diff -u -p -r1.57 sed.1
--- sed.1 14 Nov 2018 10:59:33 -0000 1.57
+++ sed.1 7 Dec 2018 07:31:54 -0000
@@ -102,10 +102,6 @@ Edit files in place, saving backups with
 If a zero length
 .Ar extension
 is given, no backup will be saved.
-It is not recommended to give a zero length
-.Ar extension
-when in place editing files, as it risks corruption or partial content
-in situations where disk space is exhausted, etc.
 In
 .Fl i
 mode, the hold space, line numbers, and ranges are reset between files.

Reply | Threaded
Open this post in threaded view
|

Re: sed(1) make -i behave a little nicer

patrick keshishian-2
On Fri, Dec 07, 2018 at 08:41:21AM +0100, Martijn van Duren wrote:

> I ran into a few minor nuisances with sed's -i mode, which are mostly
> compatible with gnu sed, but I reckon we should address.
>
> The problem is sed works by writing the output to a second file in the
> same directory as the original and after completion renaming the file
> to the original. This has two disadvantages:
> 1) The inode number changes, resulting in loss of carefully crafted
>    hardlinks.
> 2) We require to have write permission in the same directory as the
>    original file, even if we don't want to have a backup file.
>
> Diff below tries to solve this by doing the following.
> Copy the file to the backup location (/tmp/sedXXXXXXXXXX if no
> extension) and use the backup as the infile and the original as the
> outfile.
>
> Furthermore I changed the lstat to fstat, so we can edit symlinks (gsed
> supports symlinks by replacing the symlink by a new real file, which is
> also fun), and I extended the warning messages in process to show the
> backup file if we crash during operation, so people will always know
> where to recover the file in case of disaster.
>
> Because process also error(FATAL, ...)s during process and we always
> have a backup file I don't think the warning in sed.1 is worth keeping.
>
> The only downside to this new approach (that I can think of) is that we
> now temporarily have a file that is in an inconsistent state, but that's
> not much different to writing a file with any other editor.
>
> $ echo test > /usr/obj/test && dd if=/dev/zero of=/usr/obj/bloat
> /usr/obj: write failed, file system is full
> dd: /usr/obj/bloat: No space left on device
> 614113+0 records in
> 614112+0 records out
> 314425344 bytes transferred in 2.206 secs (142470196 bytes/sec)
> $ ./obj/sed -i -f /tmp/test.sed /usr/obj/test                                                                      
>
> /usr/obj: write failed, file system is full
> sed: /usr/obj/test: No space left on device (backup at /tmp/sedgRPSLImG9N)
> $ cat /tmp/test.sed
> s/test/aaa<repeat to infinity>/
> $ cat /tmp/sedgRPSLImG9N
> test
> $ ls -i /tmp/test
> 104 /tmp/test
> $ sed -i s/test/foo/ /tmp/test
> $ ls -i /tmp/test            
> 104 /tmp/test
> $ doas touch /etc/test
> $ doas chown martijn /etc/test
> $ echo test > /etc/test
> $ sed -i s/test/foo/ /etc/test
> $ cat /etc/test
> foo
>
> The diff does change quite a few mechanics, so some scrutiny is welcome.
>
> martijn@
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/main.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 main.c
> --- main.c 6 Dec 2018 20:16:04 -0000 1.39
> +++ main.c 7 Dec 2018 07:31:54 -0000
> @@ -35,6 +35,7 @@
>  
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
> +#include <sys/param.h>
>  #include <sys/stat.h>
>  
>  #include <ctype.h>
> @@ -94,13 +95,13 @@ static int rval; /* Exit status */
>   */
>  const char *fname; /* File name. */
>  const char *outfname; /* Output file name */
> -static char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
> -static char tmpfname[PATH_MAX]; /* Temporary file name (for in-place editing) */
> +char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
>  char *inplace; /* Inplace edit file extension */
>  u_long linenum;
>  
>  static void add_compunit(enum e_cut, char *);
>  static void add_file(char *);
> +int copy(FILE *, FILE *);
>  static int next_files_have_lines(void);
>  
>  int termwidth;
> @@ -310,26 +311,46 @@ again:
>   return (NULL);
>  }
>  
> +int
> +copy(FILE *src, FILE *dst)
> +{
> + unsigned char buf[MAXBSIZE];
> + size_t r, w, tw;
> +
> + while(1) {
> + if ((r = fread(buf, sizeof(*buf), sizeof(buf), src)) == 0) {
> + if (feof(src)) {
> + if (fflush(dst) == EOF)
> + return 0;
> + return 1;
> + }
> + if (errno != EINTR)
> + return 0;
> + continue;
> + }
> + tw = 0;
> + while(tw < r) {
> + w = fwrite(buf + tw, sizeof(*buf), r - tw, dst);
> + if (w == 0) {
> + if (errno != EINTR)
> + return 0;
> + continue;
> + }
> + tw += w;
> + }
> + }
> +}
> +
>  void
>  finish_file(void)
>  {
>   if (infile != NULL) {
>   fclose(infile);
> - if (*oldfname != '\0') {
> - if (rename(fname, oldfname) != 0) {
> - warning("rename()");
> - unlink(tmpfname);
> - exit(1);
> - }
> + if (inplace != NULL) {
> + if (*oldfname == '\0')
> + unlink(oldfname);

This looks a bit odd.


>   *oldfname = '\0';
>   }
> - if (*tmpfname != '\0') {
> - if (outfile != NULL && outfile != stdout)
> - fclose(outfile);
> - outfile = NULL;
> - rename(tmpfname, fname);
> - *tmpfname = '\0';
> - }
>   outfname = NULL;
>   }
>  }
> @@ -341,7 +362,7 @@ finish_file(void)
>  int
>  mf_fgets(SPACE *sp, enum e_spflag spflag)
>  {
> - struct stat sb;
> + struct stat sb, isb;
>   size_t len;
>   char *p;
>   int c, fd;
> @@ -369,10 +390,18 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
>   outfname = "stdout";
>   break;
>   }
> + infile = fopen(fname, inplace == NULL ? "r" : "r+");
> + if (infile == NULL) {
> + warning("%s", strerror(errno));
> + rval = 1;
> + continue;
> + }
>   if (inplace != NULL) {
> - if (lstat(fname, &sb) != 0)
> + /* inplace reads from copy */
> + outfile = infile;
> + if (fstat(fileno(outfile), &sb) != 0)
>   error(FATAL, "%s: %s", fname,
> -    strerror(errno ? errno : EIO));
> +    strerror(errno));
>   if (!S_ISREG(sb.st_mode))
>   error(FATAL, "%s: %s %s", fname,
>      "in-place editing only",
> @@ -382,32 +411,48 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
>      sizeof(oldfname));
>   len = strlcat(oldfname, inplace,
>      sizeof(oldfname));
> - if (len > sizeof(oldfname))
> - error(FATAL, "%s: name too long", fname);
> - }
> - len = snprintf(tmpfname, sizeof(tmpfname), "%s/sedXXXXXXXXXX",
> -    dirname(fname));
> - if (len >= sizeof(tmpfname))
> - error(FATAL, "%s: name too long", fname);
> - if ((fd = mkstemp(tmpfname)) == -1)
> - error(FATAL, "%s: %s", fname, strerror(errno));
> - if ((outfile = fdopen(fd, "w")) == NULL) {
> - unlink(tmpfname);
> - error(FATAL, "%s", fname);
> + if (len >= sizeof(oldfname))
> + error(FATAL, "backup %s: name too long",
> +    fname);
> + (void)unlink(oldfname);
> + if ((infile = fopen(oldfname, "w+")) == NULL)
> + error(FATAL, "can't create backup "
> +    "file: %s", oldfname);
> + if (fstat(fileno(infile), &isb) != 0)
> + error(FATAL, "%s: %s", oldfname,
> +    strerror(errno));
> + if (!S_ISREG(isb.st_mode))
> + error(FATAL, "backup file %s not a "
> +    "regular file and can't be "
> +    "replaced", oldfname);
> + fchown(fileno(infile), sb.st_uid, sb.st_gid);
> + fchmod(fileno(infile), sb.st_mode & ALLPERMS);
> + } else {
> + (void)strlcpy(oldfname, "/tmp/sedXXXXXXXXXX",
> +    sizeof(oldfname));
> + if ((fd = mkstemp(oldfname)) == -1)
> + error(FATAL, "%s: %s", fname,
> +    strerror(errno));
> + if ((infile = fdopen(fd, "a+")) == NULL) {
> + unlink(oldfname);
> + error(FATAL, "%s", fname);
> + }
>   }
> - fchown(fileno(outfile), sb.st_uid, sb.st_gid);
> - fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
> - outfname = tmpfname;
> + outfname = fname;
>   linenum = 0;
>   resetstate();
> + if (!copy(outfile, infile)) {
> + unlink(oldfname);
> + error(FATAL, "%s: failed to create backup copy "
> +    "to %s: %s", fname, oldfname,
> +    strerror(errno));
> + }
> + ftruncate(fileno(outfile), 0);
> + rewind(outfile);
> + rewind(infile);
>   } else {
>   outfile = stdout;
>   outfname = "stdout";
> - }
> - if ((infile = fopen(fname, "r")) == NULL) {
> - warning("%s", strerror(errno));
> - rval = 1;
> - continue;
>   }
>   }
>  
> Index: process.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/process.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 process.c
> --- process.c 14 Nov 2018 10:59:33 -0000 1.34
> +++ process.c 7 Dec 2018 07:31:54 -0000
> @@ -75,6 +75,7 @@ static int sdone; /* If any substitutes
>   /* Iov structure for 'w' commands. */
>  static regex_t *defpreg;
>  size_t maxnsub;
> +extern char oldfname[PATH_MAX];
>  regmatch_t *match;
>  
>  #define OUT() do {\
> @@ -473,7 +474,11 @@ flush_appends(void)
>   break;
>   }
>   if (ferror(outfile))
> - error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
> + error(FATAL, "%s: %s%s%s%s", outfname,
> +    strerror(errno ? errno : EIO),
> +    *oldfname == '\0' ? "" : " (backup at ",
> +    *oldfname == '\0' ? "" : oldfname,
> +    *oldfname == '\0' ? "" : ")");
>   appendx = sdone = 0;
>  }
>  
> @@ -513,7 +518,11 @@ lputs(char *s)
>   (void)fputc('$', outfile);
>   (void)fputc('\n', outfile);
>   if (ferror(outfile))
> - error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
> + error(FATAL, "%s: %s%s%s%s", outfname,
> +    strerror(errno ? errno : EIO),
> +    *oldfname == '\0' ? "" : " (backup at ",
> +    *oldfname == '\0' ? "" : oldfname,
> +    *oldfname == '\0' ? "" : ")");
>  }
>  
>  static inline int
> Index: sed.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/sed.1,v
> retrieving revision 1.57
> diff -u -p -r1.57 sed.1
> --- sed.1 14 Nov 2018 10:59:33 -0000 1.57
> +++ sed.1 7 Dec 2018 07:31:54 -0000
> @@ -102,10 +102,6 @@ Edit files in place, saving backups with
>  If a zero length
>  .Ar extension
>  is given, no backup will be saved.
> -It is not recommended to give a zero length
> -.Ar extension
> -when in place editing files, as it risks corruption or partial content
> -in situations where disk space is exhausted, etc.
>  In
>  .Fl i
>  mode, the hold space, line numbers, and ranges are reset between files.
>

Reply | Threaded
Open this post in threaded view
|

Re: sed(1) make -i behave a little nicer

Martijn van Duren-5
On 12/7/18 4:13 PM, patrick keshishian wrote:

> On Fri, Dec 07, 2018 at 08:41:21AM +0100, Martijn van Duren wrote:
>> I ran into a few minor nuisances with sed's -i mode, which are mostly
>> compatible with gnu sed, but I reckon we should address.
>>
>> The problem is sed works by writing the output to a second file in the
>> same directory as the original and after completion renaming the file
>> to the original. This has two disadvantages:
>> 1) The inode number changes, resulting in loss of carefully crafted
>>    hardlinks.
>> 2) We require to have write permission in the same directory as the
>>    original file, even if we don't want to have a backup file.
>>
>> Diff below tries to solve this by doing the following.
>> Copy the file to the backup location (/tmp/sedXXXXXXXXXX if no
>> extension) and use the backup as the infile and the original as the
>> outfile.
>>
>> Furthermore I changed the lstat to fstat, so we can edit symlinks (gsed
>> supports symlinks by replacing the symlink by a new real file, which is
>> also fun), and I extended the warning messages in process to show the
>> backup file if we crash during operation, so people will always know
>> where to recover the file in case of disaster.
>>
>> Because process also error(FATAL, ...)s during process and we always
>> have a backup file I don't think the warning in sed.1 is worth keeping.
>>
>> The only downside to this new approach (that I can think of) is that we
>> now temporarily have a file that is in an inconsistent state, but that's
>> not much different to writing a file with any other editor.
>>
>> $ echo test > /usr/obj/test && dd if=/dev/zero of=/usr/obj/bloat
>> /usr/obj: write failed, file system is full
>> dd: /usr/obj/bloat: No space left on device
>> 614113+0 records in
>> 614112+0 records out
>> 314425344 bytes transferred in 2.206 secs (142470196 bytes/sec)
>> $ ./obj/sed -i -f /tmp/test.sed /usr/obj/test                                                                      
>>
>> /usr/obj: write failed, file system is full
>> sed: /usr/obj/test: No space left on device (backup at /tmp/sedgRPSLImG9N)
>> $ cat /tmp/test.sed
>> s/test/aaa<repeat to infinity>/
>> $ cat /tmp/sedgRPSLImG9N
>> test
>> $ ls -i /tmp/test
>> 104 /tmp/test
>> $ sed -i s/test/foo/ /tmp/test
>> $ ls -i /tmp/test            
>> 104 /tmp/test
>> $ doas touch /etc/test
>> $ doas chown martijn /etc/test
>> $ echo test > /etc/test
>> $ sed -i s/test/foo/ /etc/test
>> $ cat /etc/test
>> foo
>>
>> The diff does change quite a few mechanics, so some scrutiny is welcome.
>>
>> martijn@
>>
>> Index: main.c
>> ===================================================================
>> RCS file: /cvs/src/usr.bin/sed/main.c,v
>> retrieving revision 1.39
>> diff -u -p -r1.39 main.c
>> --- main.c 6 Dec 2018 20:16:04 -0000 1.39
>> +++ main.c 7 Dec 2018 07:31:54 -0000
>> @@ -35,6 +35,7 @@
>>  
>>  #include <sys/types.h>
>>  #include <sys/ioctl.h>
>> +#include <sys/param.h>
>>  #include <sys/stat.h>
>>  
>>  #include <ctype.h>

>>  void
>>  finish_file(void)
>>  {
>>   if (infile != NULL) {
>>   fclose(infile);
>> - if (*oldfname != '\0') {
>> - if (rename(fname, oldfname) != 0) {
>> - warning("rename()");
>> - unlink(tmpfname);
>> - exit(1);
>> - }
>> + if (inplace != NULL) {
>> + if (*oldfname == '\0')
>> + unlink(oldfname);
>
> This looks a bit odd.
That's because it's a type-O. Thanks for spotting.

I also noticed I forgot to close the outfile.
Updated diff below.

>
>
>>   *oldfname = '\0';
>>   }
>> - if (*tmpfname != '\0') {
>> - if (outfile != NULL && outfile != stdout)
>> - fclose(outfile);
>> - outfile = NULL;
>> - rename(tmpfname, fname);
>> - *tmpfname = '\0';
>> - }
>>   outfname = NULL;
>>   }

Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.39
diff -u -p -r1.39 main.c
--- main.c 6 Dec 2018 20:16:04 -0000 1.39
+++ main.c 8 Dec 2018 11:47:28 -0000
@@ -35,6 +35,7 @@
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
+#include <sys/param.h>
 #include <sys/stat.h>
 
 #include <ctype.h>
@@ -94,13 +95,13 @@ static int rval; /* Exit status */
  */
 const char *fname; /* File name. */
 const char *outfname; /* Output file name */
-static char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
-static char tmpfname[PATH_MAX]; /* Temporary file name (for in-place editing) */
+char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
 char *inplace; /* Inplace edit file extension */
 u_long linenum;
 
 static void add_compunit(enum e_cut, char *);
 static void add_file(char *);
+int copy(FILE *, FILE *);
 static int next_files_have_lines(void);
 
 int termwidth;
@@ -310,26 +311,47 @@ again:
  return (NULL);
 }
 
+int
+copy(FILE *src, FILE *dst)
+{
+ unsigned char buf[MAXBSIZE];
+ size_t r, w, tw;
+
+ while(1) {
+ if ((r = fread(buf, sizeof(*buf), sizeof(buf), src)) == 0) {
+ if (feof(src)) {
+ if (fflush(dst) == EOF)
+ return 0;
+ return 1;
+ }
+ if (errno != EINTR)
+ return 0;
+ continue;
+ }
+ tw = 0;
+ while(tw < r) {
+ w = fwrite(buf + tw, sizeof(*buf), r - tw, dst);
+ if (w == 0) {
+ if (errno != EINTR)
+ return 0;
+ continue;
+ }
+ tw += w;
+ }
+ }
+}
+
 void
 finish_file(void)
 {
  if (infile != NULL) {
  fclose(infile);
- if (*oldfname != '\0') {
- if (rename(fname, oldfname) != 0) {
- warning("rename()");
- unlink(tmpfname);
- exit(1);
- }
+ fclose(outfile);
+ if (inplace != NULL) {
+ if (*oldfname != '\0')
+ unlink(oldfname);
  *oldfname = '\0';
  }
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
  outfname = NULL;
  }
 }
@@ -341,7 +363,7 @@ finish_file(void)
 int
 mf_fgets(SPACE *sp, enum e_spflag spflag)
 {
- struct stat sb;
+ struct stat sb, isb;
  size_t len;
  char *p;
  int c, fd;
@@ -369,10 +391,18 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  outfname = "stdout";
  break;
  }
+ infile = fopen(fname, inplace == NULL ? "r" : "r+");
+ if (infile == NULL) {
+ warning("%s", strerror(errno));
+ rval = 1;
+ continue;
+ }
  if (inplace != NULL) {
- if (lstat(fname, &sb) != 0)
+ /* inplace reads from copy */
+ outfile = infile;
+ if (fstat(fileno(outfile), &sb) != 0)
  error(FATAL, "%s: %s", fname,
-    strerror(errno ? errno : EIO));
+    strerror(errno));
  if (!S_ISREG(sb.st_mode))
  error(FATAL, "%s: %s %s", fname,
     "in-place editing only",
@@ -382,32 +412,48 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
     sizeof(oldfname));
  len = strlcat(oldfname, inplace,
     sizeof(oldfname));
- if (len > sizeof(oldfname))
- error(FATAL, "%s: name too long", fname);
- }
- len = snprintf(tmpfname, sizeof(tmpfname), "%s/sedXXXXXXXXXX",
-    dirname(fname));
- if (len >= sizeof(tmpfname))
- error(FATAL, "%s: name too long", fname);
- if ((fd = mkstemp(tmpfname)) == -1)
- error(FATAL, "%s: %s", fname, strerror(errno));
- if ((outfile = fdopen(fd, "w")) == NULL) {
- unlink(tmpfname);
- error(FATAL, "%s", fname);
+ if (len >= sizeof(oldfname))
+ error(FATAL, "backup %s: name too long",
+    fname);
+ (void)unlink(oldfname);
+ if ((infile = fopen(oldfname, "w+")) == NULL)
+ error(FATAL, "can't create backup "
+    "file: %s", oldfname);
+ if (fstat(fileno(infile), &isb) != 0)
+ error(FATAL, "%s: %s", oldfname,
+    strerror(errno));
+ if (!S_ISREG(isb.st_mode))
+ error(FATAL, "backup file %s not a "
+    "regular file and can't be "
+    "replaced", oldfname);
+ fchown(fileno(infile), sb.st_uid, sb.st_gid);
+ fchmod(fileno(infile), sb.st_mode & ALLPERMS);
+ } else {
+ (void)strlcpy(oldfname, "/tmp/sedXXXXXXXXXX",
+    sizeof(oldfname));
+ if ((fd = mkstemp(oldfname)) == -1)
+ error(FATAL, "%s: %s", fname,
+    strerror(errno));
+ if ((infile = fdopen(fd, "a+")) == NULL) {
+ unlink(oldfname);
+ error(FATAL, "%s", fname);
+ }
  }
- fchown(fileno(outfile), sb.st_uid, sb.st_gid);
- fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
- outfname = tmpfname;
+ outfname = fname;
  linenum = 0;
  resetstate();
+ if (!copy(outfile, infile)) {
+ unlink(oldfname);
+ error(FATAL, "%s: failed to create backup copy "
+    "to %s: %s", fname, oldfname,
+    strerror(errno));
+ }
+ ftruncate(fileno(outfile), 0);
+ rewind(outfile);
+ rewind(infile);
  } else {
  outfile = stdout;
  outfname = "stdout";
- }
- if ((infile = fopen(fname, "r")) == NULL) {
- warning("%s", strerror(errno));
- rval = 1;
- continue;
  }
  }
 
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.34
diff -u -p -r1.34 process.c
--- process.c 14 Nov 2018 10:59:33 -0000 1.34
+++ process.c 8 Dec 2018 11:47:28 -0000
@@ -75,6 +75,7 @@ static int sdone; /* If any substitutes
  /* Iov structure for 'w' commands. */
 static regex_t *defpreg;
 size_t maxnsub;
+extern char oldfname[PATH_MAX];
 regmatch_t *match;
 
 #define OUT() do {\
@@ -473,7 +474,11 @@ flush_appends(void)
  break;
  }
  if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+    strerror(errno ? errno : EIO),
+    *oldfname == '\0' ? "" : " (backup at ",
+    *oldfname == '\0' ? "" : oldfname,
+    *oldfname == '\0' ? "" : ")");
  appendx = sdone = 0;
 }
 
@@ -513,7 +518,11 @@ lputs(char *s)
  (void)fputc('$', outfile);
  (void)fputc('\n', outfile);
  if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+    strerror(errno ? errno : EIO),
+    *oldfname == '\0' ? "" : " (backup at ",
+    *oldfname == '\0' ? "" : oldfname,
+    *oldfname == '\0' ? "" : ")");
 }
 
 static inline int
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.57
diff -u -p -r1.57 sed.1
--- sed.1 14 Nov 2018 10:59:33 -0000 1.57
+++ sed.1 8 Dec 2018 11:47:28 -0000
@@ -102,10 +102,6 @@ Edit files in place, saving backups with
 If a zero length
 .Ar extension
 is given, no backup will be saved.
-It is not recommended to give a zero length
-.Ar extension
-when in place editing files, as it risks corruption or partial content
-in situations where disk space is exhausted, etc.
 In
 .Fl i
 mode, the hold space, line numbers, and ranges are reset between files.