[Emacs-diffs] master 5fafa40: Improve checking of pdump load failures

[Paul Eggert]

branch: master
commit 5fafa40d076ee24baf880e97d4290b6196cf838a
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Improve checking of pdump load failures
    
    * src/alloc.c (memory_full): Just report "memory exhausted" if
    failure occurs during initialization, since fancier recovery
    schemes are not likely to work when not initialized.
    * src/emacs.c (dump_error_to_string): Accept int, not enum
    pdumper_load_result, since the result might not fit in the
    enum.  Use strerror if it was derived from errno.  This is for
    better diagnostics of pdump load failures.
    (load_pdump_find_executable): Return char *, not enum.  2nd
    arg is now pointer to buffer size, rather than pointer to
    pointer to buffer.  All callers changed.  Use Emacs allocator
    since they should now be OK even during early startup.
    Use check_executable instead access, to use effective rather
    than real permissions.
    (load_pdump): Return void since callers ignore result.
    Use int where enum could be too narrow.  Use heap rather
    than stack for possibly-long string.  Prefer ptrdiff_t to
    size_t.
    * src/fileio.c (check_executable): Now extern.
    * src/pdumper.c (pdumper_load): Return int that may have
    errno added to it, for better diagnostics when loads fail.
---
 src/alloc.c   |   3 +
 src/emacs.c   | 182 ++++++++++++++++++++++++----------------------------------
 src/fileio.c  |   2 +-
 src/lisp.h    |   1 +
 src/pdumper.c |  11 +++-
 src/pdumper.h |   4 +-
 6 files changed, 91 insertions(+), 112 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index be98cfd..2d490f3 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3844,6 +3844,9 @@ set_interval_marked (INTERVAL i)
 void
 memory_full (size_t nbytes)
 {
+  if (!initialized)
+    fatal ("memory exhausted");
+
   /* Do not go into hysterics merely because a large request failed.  */
   bool enough_free_memory = false;
   if (SPARE_MEMORY < nbytes)
diff --git a/src/emacs.c b/src/emacs.c
index 53572d7..5a52668 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -662,7 +662,7 @@ argmatch (char **argv, int argc, const char *sstr, const 
char *lstr,
 #ifdef HAVE_PDUMPER
 
 static const char *
-dump_error_to_string (enum pdumper_load_result result)
+dump_error_to_string (int result)
 {
   switch (result)
     {
@@ -681,37 +681,29 @@ dump_error_to_string (enum pdumper_load_result result)
     case PDUMPER_LOAD_VERSION_MISMATCH:
       return "not built for this Emacs executable";
     default:
-      return "generic error";
+      return (result <= PDUMPER_LOAD_ERROR
+             ? "generic error"
+             : strerror (result - PDUMPER_LOAD_ERROR));
     }
 }
 
-/* Find a path (absolute or relative) to the Emacs executable.
-   Called early in initialization by portable dumper loading code, so we
-   can't use lisp and associated machinery.  On success, *EXENAME is
-   set to a heap-allocated string giving a path to the Emacs
-   executable or to NULL if we can't determine the path immediately.
- */
-static enum pdumper_load_result
-load_pdump_find_executable (const char* argv0, char **exename)
+/* Find a name (absolute or relative) of the Emacs executable whose
+   name (as passed into this program) is ARGV0.  Called early in
+   initialization by portable dumper loading code, so avoid Lisp and
+   associated machinery.  Return a heap-allocated string giving a name
+   of the Emacs executable, or an empty heap-allocated string or NULL
+   if not found.  Store into *CANDIDATE_SIZE a lower bound on the size
+   of any heap allocation.  */
+static char *
+load_pdump_find_executable (char const *argv0, ptrdiff_t *candidate_size)
 {
-  enum pdumper_load_result result;
+  *candidate_size = 0;
+
+  /* Use xstrdup etc. to allocate storage, so as to call our private
+     implementation of malloc, since the caller calls our free.  */
 #ifdef WINDOWSNT
-  result = PDUMPER_LOAD_ERROR;
-  *exename = NULL;
   char *prog_fname = w32_my_exename ();
-  if (prog_fname)
-    {
-      result = PDUMPER_LOAD_OOM;
-      /* Use xstrdup, so as to call our private implementation of
-        malloc, since the caller calls our free.  */
-      char *ret = xstrdup (prog_fname);
-      if (ret)
-       {
-         *exename = ret;
-         result = PDUMPER_LOAD_SUCCESS;
-       }
-    }
-  return result;
+  return prog_fname ? xstrdup (prog_fname) : NULL;
 #else  /* !WINDOWSNT */
   char *candidate = NULL;
 
@@ -719,33 +711,23 @@ load_pdump_find_executable (const char* argv0, char 
**exename)
      path already, so just copy it.  */
   eassert (argv0);
   if (strchr (argv0, DIRECTORY_SEP))
-    {
-      result = PDUMPER_LOAD_OOM;
-      char *ret = strdup (argv0);
-      if (!ret)
-        goto out;
-      result = PDUMPER_LOAD_SUCCESS;
-      *exename = ret;
-      goto out;
-    }
-  size_t argv0_length = strlen (argv0);
+    return xstrdup (argv0);
+  ptrdiff_t argv0_length = strlen (argv0);
 
   const char *path = getenv ("PATH");
   if (!path)
     {
       /* Default PATH is implementation-defined, so we don't know how
          to conduct the search.  */
-      result = PDUMPER_LOAD_SUCCESS;
-      *exename = NULL;
-      goto out;
+      return NULL;
     }
 
   /* Actually try each concatenation of a path element and the
      executable basename.  */
-  const char path_sep[] = { SEPCHAR, '\0' };
   do
     {
-      size_t path_part_length = strcspn (path, path_sep);
+      static char const path_sep[] = { SEPCHAR, '\0' };
+      ptrdiff_t path_part_length = strcspn (path, path_sep);
       const char *path_part = path;
       path += path_part_length;
       if (path_part_length == 0)
@@ -753,46 +735,34 @@ load_pdump_find_executable (const char* argv0, char 
**exename)
           path_part = ".";
           path_part_length = 1;
         }
-      size_t candidate_length = path_part_length + 1 + argv0_length;
-      {
-        char *new_candidate = realloc (candidate, candidate_length + 1);
-        if (!new_candidate)
-          {
-            result = PDUMPER_LOAD_OOM;
-            goto out;
-          }
-        candidate = new_candidate;
-      }
+      ptrdiff_t needed = path_part_length + 1 + argv0_length + 1;
+      if (*candidate_size <= needed)
+       {
+         xfree (candidate);
+         candidate = xpalloc (NULL, candidate_size,
+                              needed - *candidate_size + 1, -1, 1);
+       }
       memcpy (candidate + 0, path_part, path_part_length);
       candidate[path_part_length] = DIRECTORY_SEP;
       memcpy (candidate + path_part_length + 1, argv0, argv0_length + 1);
       struct stat st;
-      if (!access (candidate, X_OK) &&
-          !stat (candidate, &st) &&
-          S_ISREG (st.st_mode))
-        {
-          *exename = candidate;
-          candidate = NULL;
-          break;
-        }
-    } while ((path++)[0] != '\0');
-
-  result = PDUMPER_LOAD_SUCCESS;
+      if (check_executable (candidate)
+         && stat (candidate, &st) == 0 && S_ISREG (st.st_mode))
+       return candidate;
+      *candidate = '\0';
+    }
+  while (*path++ != '\0');
 
- out:
-  free (candidate);
-  return result;
+  return candidate;
 #endif /* !WINDOWSNT */
 }
 
-static enum pdumper_load_result
+static void
 load_pdump (int argc, char **argv)
 {
   const char *const suffix = ".pdmp";
-  enum pdumper_load_result result;
-  char *exename = NULL;
-  char *real_exename = NULL;
-  const char* strip_suffix =
+  int result;
+  const char *strip_suffix =
 #if defined DOS_NT || defined CYGWIN
     ".exe"
 #else
@@ -821,7 +791,6 @@ load_pdump (int argc, char **argv)
       skip_args++;
     }
 
-  result = PDUMPER_NOT_LOADED;
   if (dump_file)
     {
       result = pdumper_load (dump_file);
@@ -829,8 +798,7 @@ load_pdump (int argc, char **argv)
       if (result != PDUMPER_LOAD_SUCCESS)
         fatal ("could not load dump file \"%s\": %s",
                dump_file, dump_error_to_string (result));
-      else
-        goto out;
+      return;
     }
 
   /* Look for a dump file in the same directory as the executable; it
@@ -839,44 +807,41 @@ load_pdump (int argc, char **argv)
      so we can't use decode_env_path.  We're working in whatever
      encoding the system natively uses for filesystem access, so
      there's no need for character set conversion.  */
-  result = load_pdump_find_executable (argv[0], &exename);
-  if (result != PDUMPER_LOAD_SUCCESS)
-    goto out;
+  ptrdiff_t bufsize;
+  dump_file = load_pdump_find_executable (argv[0], &bufsize);
 
   /* If we couldn't find our executable, go straight to looking for
      the dump in the hardcoded location.  */
-  if (exename)
+  if (dump_file && *dump_file)
     {
 #ifdef WINDOWSNT
       /* w32_my_exename resolves symlinks internally, so no need to
         call realpath.  */
-      real_exename = exename;
-      exename = NULL;
 #else
-      real_exename = realpath (exename, NULL);
+      char *real_exename = realpath (dump_file, NULL);
       if (!real_exename)
         fatal ("could not resolve realpath of \"%s\": %s",
-               exename, strerror (errno));
+               dump_file, strerror (errno));
+      xfree (dump_file);
+      dump_file = real_exename;
+#endif
+      ptrdiff_t exenamelen = strlen (dump_file);
+#ifndef WINDOWSNT
+      bufsize = exenamelen + 1;
 #endif
-      size_t real_exename_length = strlen (real_exename);
       if (strip_suffix)
         {
-          size_t strip_suffix_length = strlen (strip_suffix);
-          if (real_exename_length >= strip_suffix_length)
-            {
-              size_t prefix_length =
-                real_exename_length - strip_suffix_length;
-              if (!memcmp (&real_exename[prefix_length],
-                           strip_suffix,
-                           strip_suffix_length))
-                  real_exename_length = prefix_length;
-            }
+         ptrdiff_t strip_suffix_length = strlen (strip_suffix);
+         ptrdiff_t prefix_length = exenamelen - strip_suffix_length;
+         if (0 <= prefix_length
+             && !memcmp (&dump_file[prefix_length], strip_suffix,
+                         strip_suffix_length))
+           exenamelen = prefix_length;
         }
-      dump_file = alloca (real_exename_length + strlen (suffix) + 1);
-      memcpy (dump_file, real_exename, real_exename_length);
-      memcpy (dump_file + real_exename_length,
-              suffix,
-              strlen (suffix) + 1);
+      ptrdiff_t needed = exenamelen + strlen (suffix) + 1;
+      if (bufsize < needed)
+       dump_file = xpalloc (dump_file, &bufsize, needed - bufsize, -1, 1);
+      strcpy (dump_file + exenamelen, suffix);
       result = pdumper_load (dump_file);
       if (result == PDUMPER_LOAD_SUCCESS)
         goto out;
@@ -896,16 +861,19 @@ load_pdump (int argc, char **argv)
      "emacs.pdmp" so that the Emacs binary still works if the user
      copies and renames it.  */
   const char *argv0_base = "emacs";
-  dump_file = alloca (strlen (path_exec)
+  ptrdiff_t needed = (strlen (path_exec)
                       + 1
                       + strlen (argv0_base)
                       + strlen (suffix)
                       + 1);
+  if (bufsize < needed)
+    {
+      xfree (dump_file);
+      dump_file = xpalloc (NULL, &bufsize, needed - bufsize, -1, 1);
+    }
   sprintf (dump_file, "%s%c%s%s",
            path_exec, DIRECTORY_SEP, argv0_base, suffix);
   result = pdumper_load (dump_file);
-  if (result == PDUMPER_LOAD_SUCCESS)
-    goto out;
 
   if (result == PDUMPER_LOAD_FILE_NOT_FOUND)
     {
@@ -920,13 +888,18 @@ load_pdump (int argc, char **argv)
            last_sep = p;
        }
       argv0_base = last_sep ? last_sep + 1 : argv[0];
-      dump_file = alloca (strlen (path_exec)
+      ptrdiff_t needed = (strlen (path_exec)
                          + 1
                          + strlen (argv0_base)
                          + strlen (suffix)
                          + 1);
+      if (bufsize < needed)
+       {
+         xfree (dump_file);
+         dump_file = xmalloc (needed);
+       }
 #ifdef DOS_NT
-      size_t argv0_len = strlen (argv0_base);
+      ptrdiff_t argv0_len = strlen (argv0_base);
       if (argv0_len >= 4
          && c_strcasecmp (argv0_base + argv0_len - 4, ".exe") == 0)
        sprintf (dump_file, "%s%c%.*s%s", path_exec, DIRECTORY_SEP,
@@ -943,13 +916,10 @@ load_pdump (int argc, char **argv)
       if (result != PDUMPER_LOAD_FILE_NOT_FOUND)
        fatal ("could not load dump file \"%s\": %s",
               dump_file, dump_error_to_string (result));
-      dump_file = NULL;
     }
 
  out:
-  free (exename);
-  free (real_exename);
-  return result;
+  xfree (dump_file);
 }
 #endif /* HAVE_PDUMPER */
 
diff --git a/src/fileio.c b/src/fileio.c
index 968a55e..cbc0c89 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -150,7 +150,7 @@ check_existing (const char *filename)
 
 /* Return true if file FILENAME exists and can be executed.  */
 
-static bool
+bool
 check_executable (char *filename)
 {
   return faccessat (AT_FDCWD, filename, X_OK, AT_EACCESS) == 0;
diff --git a/src/lisp.h b/src/lisp.h
index a7b19ab..024e5ed 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4298,6 +4298,7 @@ extern void syms_of_marker (void);
 
 /* Defined in fileio.c.  */
 
+extern bool check_executable (char *);
 extern char *splice_dir_file (char *, char const *, char const *);
 extern bool file_name_absolute_p (const char *);
 extern char const *get_homedir (void);
diff --git a/src/pdumper.c b/src/pdumper.c
index 9809023..2e38214 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -5303,7 +5303,7 @@ enum dump_section
 
    N.B. We run very early in initialization, so we can't use lisp,
    unwinding, xmalloc, and so on.  */
-enum pdumper_load_result
+int
 pdumper_load (const char *dump_filename)
 {
   intptr_t dump_size;
@@ -5328,10 +5328,15 @@ pdumper_load (const char *dump_filename)
   /* We can load only one dump.  */
   eassert (!dump_loaded_p ());
 
-  enum pdumper_load_result err = PDUMPER_LOAD_FILE_NOT_FOUND;
+  int err;
   int dump_fd = emacs_open (dump_filename, O_RDONLY, 0);
   if (dump_fd < 0)
-    goto out;
+    {
+      err = (errno == ENOENT || errno == ENOTDIR
+            ? PDUMPER_LOAD_FILE_NOT_FOUND
+            : PDUMPER_LOAD_ERROR + errno);
+      goto out;
+    }
 
   err = PDUMPER_LOAD_FILE_NOT_FOUND;
   if (fstat (dump_fd, &stat) < 0)
diff --git a/src/pdumper.h b/src/pdumper.h
index 83c094f..31b0d53 100644
--- a/src/pdumper.h
+++ b/src/pdumper.h
@@ -124,10 +124,10 @@ enum pdumper_load_result
     PDUMPER_LOAD_FAILED_DUMP,
     PDUMPER_LOAD_OOM,
     PDUMPER_LOAD_VERSION_MISMATCH,
-    PDUMPER_LOAD_ERROR,
+    PDUMPER_LOAD_ERROR /* Must be last, as errno may be added.  */
   };
 
-enum pdumper_load_result pdumper_load (const char *dump_filename);
+int pdumper_load (const char *dump_filename);
 
 struct pdumper_loaded_dump
 {