sed: fix temp file cleanup

Without this fix, the code would sometimes use FP after calling
fclose (FP), which has undefined behavior in C.
Problem found with --enable-gcc-warnings and GCC 12.
* sed/execute.c (open_next_file): Do not register here,
as it’s too late and this can cause the file to not
be cleaned up.
* sed/sed.c (G_file_to_unlink, register_cleanup_file, cancel_cleanup):
Move from here to utils.c.
(cleanup): Call remove_cleanup_file instead of doing it by hand.
* sed/utils.c (struct open_file): Remove member temp
(which was always false) and fclose_failed (which was
not enough to prevent calling fclose with a bad pointer).
All uses changed.
(register_open_file): Do not access p->fp after it’s fclosed,
as that has undefined behavior in C.
Use xmalloc instead of xcalloc, since we initialize all members.
(G_file_to_unlink, register_cleanup_file, cancel_cleanup):
Move from utils.c to here.
(remove_cleanup_file): New function.
(ck_mkstemp): Fix a screwup when mkostemp succeeded but
set_binary_mode or fdopen failed: we might misuse a null pointer,
or forget to clean up the newly-created temp file.
(ck_getdelim): Rename local to avoid confusion with global.
(mark_as_fclose_failed): Remove.  All uses removed.
(ck_fclose): Remove entry from open_files before attempting
to fclose it, so that panicking doesn’t try to fclose it again.
(do_ck_fclose): New arg NAME so that there’s no need to
call mark_as_fclose_failed, which inspected FP after fclosing
it, which is undefined behavior.
(ck_rename): Omit arg UNLINK_IF_FAIL.  All callers changed.
The cleanup handler removes this file now, as needed.

6 files changed, 75 insertions, 123 deletions

diff --git a/NEWS b/NEWS
index 47d9c2c..e7996b3 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,9 @@ GNU sed NEWS                                    -*- outline -*-
   Private Use Area plane B).
   [bug introduced in sed 4.8]
 
+  I/O errors involving temp files no longer confuse sed into using a
+  FILE * pointer after fclosing it, which has undefined behavior in C.
+
 ** New Features
 
   The 'r' command now accepts address 0, allowing inserting a file before
diff --git a/sed/execute.c b/sed/execute.c
index 127f67d..485bca7 100644
--- a/sed/execute.c
+++ b/sed/execute.c
@@ -104,7 +104,7 @@ struct input {
 /* Have we done any replacements lately?  This is used by the `t' command. */
 static bool replaced = false;
 
-/* The current output file (stdout if -i is not being used. */
+/* The current output file (stdout if -i is not being used).  */
 static struct output output_file;
 
 /* The `current' input line. */
@@ -620,7 +620,6 @@ open_next_file (const char *name, struct input *input)
 
       output_file.fp = ck_mkstemp (&input->out_file_name, tmpdir, "sed",
                                    write_mode);
-      register_cleanup_file (input->out_file_name);
       output_file.missing_newline = false;
       free (tmpdir);
 
@@ -674,11 +673,11 @@ closedown (struct input *input)
       if (strcmp (in_place_extension, "*") != 0)
         {
           char *backup_file_name = get_backup_file_name (target_name);
-          ck_rename (target_name, backup_file_name, input->out_file_name);
+          ck_rename (target_name, backup_file_name);
           free (backup_file_name);
         }
 
-      ck_rename (input->out_file_name, target_name, input->out_file_name);
+      ck_rename (input->out_file_name, target_name);
       cancel_cleanup ();
       free (input->out_file_name);
     }
diff --git a/sed/sed.c b/sed/sed.c
index 1678254..42c6a1f 100644
--- a/sed/sed.c
+++ b/sed/sed.c
@@ -85,12 +85,6 @@ countT lcmd_out_line_len = 70;
 /* The complete compiled SED program that we are going to run: */
 static struct vector *the_program = NULL;
 
-/* When we've created a temporary for an in-place update,
-   we may have to exit before the rename.  This is the name
-   of the temporary that we'll have to unlink via an atexit-
-   registered cleanup function.  */
-static char const *G_file_to_unlink;
-
 struct localeinfo localeinfo;
 
 /* When exiting between temporary file creation and the rename
@@ -99,22 +93,7 @@ static void
 cleanup (void)
 {
   IF_LINT (free (in_place_extension));
-  if (G_file_to_unlink)
-    unlink (G_file_to_unlink);
-}
-
-/* Note that FILE must be removed upon exit.  */
-void
-register_cleanup_file (char const *file)
-{
-  G_file_to_unlink = file;
-}
-
-/* Clear the global file-to-unlink global.  */
-void
-cancel_cleanup (void)
-{
-  G_file_to_unlink = NULL;
+  remove_cleanup_file ();
 }
 
 static void
diff --git a/sed/sed.h b/sed/sed.h
index f1b0156..1c96bc5 100644
--- a/sed/sed.h
+++ b/sed/sed.h
@@ -279,8 +279,6 @@ extern bool debug;
 
 extern int is_mb_char (int ch, mbstate_t *ps);
 extern void initialize_mbcs (void);
-extern void register_cleanup_file (char const *file);
-extern void cancel_cleanup (void);
 
 /* Use this to suppress gcc's '...may be used before initialized' warnings. */
 #ifdef lint
diff --git a/sed/utils.c b/sed/utils.c
index 5765dc3..823de8d 100644
--- a/sed/utils.c
+++ b/sed/utils.c
@@ -46,12 +46,10 @@ struct open_file
     FILE *fp;
     char *name;
     struct open_file *link;
-    unsigned temp : 1;
-    unsigned fclose_failed : 1;
   };
 
 static struct open_file *open_files = NULL;
-static void do_ck_fclose (FILE *fp);
+static void do_ck_fclose (FILE *, char const *);
 
 /* Print an error message and exit */
 
@@ -66,29 +64,15 @@ panic (const char *str, ...)
   va_end (ap);
   putc ('\n', stderr);
 
-  /* Unlink the temporary files.  */
+#ifdef lint
   while (open_files)
     {
-      if (open_files->temp)
-        {
-          if (!open_files->fclose_failed)
-            fclose (open_files->fp);
-          errno = 0;
-          unlink (open_files->name);
-          if (errno != 0)
-            fprintf (stderr, _("cannot remove %s: %s"), open_files->name,
-                     strerror (errno));
-        }
-
-#ifdef lint
       struct open_file *next = open_files->link;
       free (open_files->name);
       free (open_files);
       open_files = next;
-#else
-      open_files = open_files->link;
-#endif
     }
+#endif
 
   exit (EXIT_PANIC);
 }
@@ -116,24 +100,11 @@ static void
 register_open_file (FILE *fp, const char *name)
 {
   struct open_file *p;
-  for (p=open_files; p; p=p->link)
-    {
-      if (fp == p->fp)
-        {
-          free (p->name);
-          break;
-        }
-    }
-  if (!p)
-    {
-      p = XCALLOC (1, struct open_file);
-      p->link = open_files;
-      open_files = p;
-    }
+  p = xmalloc (sizeof *p);
+  p->link = open_files;
+  open_files = p;
   p->name = xstrdup (name);
   p->fp = fp;
-  p->temp = false;
-  p->fclose_failed = false;
 }
 
 /* Panic on failing fopen */
@@ -174,6 +145,33 @@ ck_fdopen ( int fd, const char *name, const char *mode, int fail)
   return fp;
 }
 
+/* When we've created a temporary for an in-place update,
+   we may have to exit before the rename.  This is the name
+   of the temporary that we'll have to unlink via an atexit-
+   registered cleanup function.  */
+static char const *G_file_to_unlink;
+
+void
+remove_cleanup_file (void)
+{
+  if (G_file_to_unlink)
+    unlink (G_file_to_unlink);
+}
+
+/* Note that FILE must be removed upon exit.  */
+static void
+register_cleanup_file (char const *file)
+{
+  G_file_to_unlink = file;
+}
+
+/* Clear the global file-to-unlink global.  */
+void
+cancel_cleanup (void)
+{
+  G_file_to_unlink = NULL;
+}
+
 FILE *
 ck_mkstemp (char **p_filename, const char *tmpdir,
             const char *base, const char *mode)
@@ -186,17 +184,28 @@ ck_mkstemp (char **p_filename, const char *tmpdir,
       mkstemp forces O_BINARY on cygwin, so use mkostemp instead.  */
   mode_t save_umask = umask (0077);
   int fd = mkostemp (template, 0);
+  int err = errno;
   umask (save_umask);
-  if (fd == -1)
-    panic (_("couldn't open temporary file %s: %s"), template,
-           strerror (errno));
+  FILE *fp = NULL;
+
+  if (0 <= fd)
+    {
+      *p_filename = template;
+      register_cleanup_file (template);
+
 #if O_BINARY
-  if (binary_mode && (set_binary_mode ( fd, O_BINARY) == -1))
-      panic (_("failed to set binary mode on '%s'"), template);
+      if (binary_mode && set_binary_mode (fd, O_BINARY) == -1)
+        panic (_("failed to set binary mode on '%s'"), template);
 #endif
 
-  *p_filename = template;
-  FILE *fp = fdopen (fd, mode);
+      fp = fdopen (fd, mode);
+      err = errno;
+    }
+
+  if (!fp)
+    panic (_("couldn't open temporary file %s: %s"), template,
+           strerror (err));
+
   register_open_file (fp, template);
   return fp;
 }
@@ -225,7 +234,7 @@ ck_fread (void *ptr, size_t size, size_t nmemb, FILE *stream)
 }
 
 size_t
-ck_getdelim (char **text, size_t *buflen, char buffer_delimiter, FILE *stream)
+ck_getdelim (char **text, size_t *buflen, char delim, FILE *stream)
 {
   ssize_t result;
   bool error;
@@ -233,7 +242,7 @@ ck_getdelim (char **text, size_t *buflen, char buffer_delimiter, FILE *stream)
   error = ferror (stream);
   if (!error)
     {
-      result = getdelim (text, buflen, buffer_delimiter, stream);
+      result = getdelim (text, buflen, delim, stream);
       error = ferror (stream);
     }
 
@@ -255,69 +264,44 @@ ck_fflush (FILE *stream)
     panic ("couldn't flush %s: %s", utils_fp_name (stream), strerror (errno));
 }
 
-/* If we've failed to close a file in open_files whose "fp" member
-   is the same as FP, mark its entry as fclose_failed.  */
-static void
-mark_as_fclose_failed (FILE *fp)
-{
-  for (struct open_file *p = open_files; p; p = p->link)
-    {
-      if (p->fp == fp)
-        {
-          p->fclose_failed = true;
-          break;
-        }
-    }
-}
-
 /* Panic on failing fclose */
 void
 ck_fclose (FILE *stream)
 {
-  struct open_file r;
-  struct open_file *prev;
+  struct open_file **prev = &open_files;
   struct open_file *cur;
 
   /* a NULL stream means to close all files */
-  r.link = open_files;
-  prev = &r;
-  while ( (cur = prev->link) )
+  while ((cur = *prev))
     {
       if (!stream || stream == cur->fp)
         {
-          do_ck_fclose (cur->fp);
-          prev->link = cur->link;
+          FILE *fp = cur->fp;
+          *prev = cur->link;
+          do_ck_fclose (fp, cur->name);
           free (cur->name);
           free (cur);
         }
       else
-        prev = cur;
+        prev = &cur->link;
     }
 
-  open_files = r.link;
-
   /* Also care about stdout, because if it is redirected the
      last output operations might fail and it is important
      to signal this as an error (perhaps to make). */
   if (!stream)
-    do_ck_fclose (stdout);
+    do_ck_fclose (stdout, "stdout");
 }
 
 /* Close a single file. */
-void
-do_ck_fclose (FILE *fp)
+static void
+do_ck_fclose (FILE *fp, char const *name)
 {
   ck_fflush (fp);
   clearerr (fp);
 
   if (fclose (fp) == EOF)
-    {
-      /* Mark as already fclose-failed, so we don't attempt to fclose it
-         a second time via panic.  */
-      mark_as_fclose_failed (fp);
-
-      panic ("couldn't close %s: %s", utils_fp_name (fp), strerror (errno));
-    }
+    panic ("couldn't close %s: %s", name, strerror (errno));
 }
 
 /* Follow symlink and panic if something fails.  Return the ultimate
@@ -401,26 +385,12 @@ follow_symlink (const char *fname)
 
 /* Panic on failing rename */
 void
-ck_rename (const char *from, const char *to, const char *unlink_if_fail)
+ck_rename (const char *from, const char *to)
 {
   int rd = rename (from, to);
   if (rd != -1)
     return;
 
-  if (unlink_if_fail)
-    {
-      int save_errno = errno;
-      errno = 0;
-      unlink (unlink_if_fail);
-
-      /* Failure to remove the temporary file is more severe,
-         so trigger it first.  */
-      if (errno != 0)
-        panic (_("cannot remove %s: %s"), unlink_if_fail, strerror (errno));
-
-      errno = save_errno;
-    }
-
   panic (_("cannot rename %s: %s"), from, strerror (errno));
 }
 
diff --git a/sed/utils.h b/sed/utils.h
index e3a8532..cac8a05 100644
--- a/sed/utils.h
+++ b/sed/utils.h
@@ -40,11 +40,14 @@ size_t ck_getdelim (char **text, size_t *buflen, char buffer_delimiter,
                     FILE *stream);
 FILE * ck_mkstemp (char **p_filename, const char *tmpdir, const char *base,
                    const char *mode) _GL_ARG_NONNULL ((1, 2, 3, 4));
-void ck_rename (const char *from, const char *to, const char *unlink_if_fail);
+void ck_rename (const char *from, const char *to);
 
 void *ck_malloc (size_t size);
 void *ck_realloc (void *ptr, size_t size);
 
+void cancel_cleanup (void);
+void remove_cleanup_file (void);
+
 struct buffer *init_buffer (void);
 char *get_buffer (struct buffer const *b) _GL_ATTRIBUTE_PURE;
 size_t size_buffer (struct buffer const *b) _GL_ATTRIBUTE_PURE;