* filelock.c: Fix some buffer overrun and integer overflow issues.

(get_boot_time): Don't assume that gzip command string fits in 100 bytes.
Reformulate so as not to need the command string.
Invoke gzip -cd rather than gunzip, as it's more portable.
(lock_info_type, lock_file_1, lock_file):
Don't assume pid_t and time_t fit in unsigned long.
(LOCK_PID_MAX): Remove; we now use more-reliable bounds.
(current_lock_owner): Prefer signed type for sizes.
Use memcpy, not strncpy, where memcpy is what is really wanted.
Don't assume (via atoi) that time_t and pid_t fit in int.
Check for time_t and/or pid_t out of range, e.g., via a network share.
Don't alloca where an auto var works fine.

diff --git a/src/ChangeLog b/src/ChangeLog
index 3687da81fbb9552e41111652ca1c9d7fc9f458f6..fd17f85f1e1102e85475f14be1a007c7f0fbd68c 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,18 @@
+2011-06-20  Paul Eggert  <eggert@cs.ucla.edu>
+
+       * filelock.c: Fix some buffer overrun and integer overflow issues.
+       (get_boot_time): Don't assume that gzip command string fits in 100 bytes.
+       Reformulate so as not to need the command string.
+       Invoke gzip -cd rather than gunzip, as it's more portable.
+       (lock_info_type, lock_file_1, lock_file):
+       Don't assume pid_t and time_t fit in unsigned long.
+       (LOCK_PID_MAX): Remove; we now use more-reliable bounds.
+       (current_lock_owner): Prefer signed type for sizes.
+       Use memcpy, not strncpy, where memcpy is what is really wanted.
+       Don't assume (via atoi) that time_t and pid_t fit in int.
+       Check for time_t and/or pid_t out of range, e.g., via a network share.
+       Don't alloca where an auto var works fine.
+
 2011-06-19  Paul Eggert  <eggert@cs.ucla.edu>
 
        * fileio.c: Fix some integer overflow issues.

diff --git a/src/filelock.c b/src/filelock.c
index 13b27c72f1921a992ec73d7c067846b30baa989d..18483b6f3f3875492c16441e88c48ae1d8c27f71 100644 (file)
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -168,7 +168,7 @@ get_boot_time (void)
   /* If we did not find a boot time in wtmp, look at wtmp, and so on.  */
   for (counter = 0; counter < 20 && ! boot_time; counter++)
     {
-      char cmd_string[100];
+      char cmd_string[sizeof WTMP_FILE ".19.gz"];
       Lisp_Object tempname, filename;
       int delete_flag = 0;
 
@@ -191,19 +191,16 @@ get_boot_time (void)
                 character long prefix, and call make_temp_file with
                 second arg non-zero, so that it will add not more
                 than 6 characters to the prefix.  */
-             tempname = Fexpand_file_name (build_string ("wt"),
+             filename = Fexpand_file_name (build_string ("wt"),
                                            Vtemporary_file_directory);
-             tempname = make_temp_name (tempname, 1);
-             args[0] = Vshell_file_name;
+             filename = make_temp_name (filename, 1);
+             args[0] = build_string ("gzip");
              args[1] = Qnil;
-             args[2] = Qnil;
+             args[2] = list2 (QCfile, filename);
              args[3] = Qnil;
-             args[4] = build_string ("-c");
-             sprintf (cmd_string, "gunzip < %s.%d.gz > %s",
-                      WTMP_FILE, counter, SDATA (tempname));
-             args[5] = build_string (cmd_string);
+             args[4] = build_string ("-cd");
+             args[5] = tempname;
              Fcall_process (6, args);
-             filename = tempname;
              delete_flag = 1;
            }
        }
@@ -284,14 +281,10 @@ typedef struct
 {
   char *user;
   char *host;
-  unsigned long pid;
+  pid_t pid;
   time_t boot_time;
 } lock_info_type;
 
-/* When we read the info back, we might need this much more,
-   enough for decimal representation plus null.  */
-#define LOCK_PID_MAX (4 * sizeof (unsigned long))
-
 /* Free the two dynamically-allocated pieces in PTR.  */
 #define FREE_LOCK_INFO(i) do { xfree ((i).user); xfree ((i).host); } while (0)
 
@@ -344,7 +337,7 @@ static int
 lock_file_1 (char *lfname, int force)
 {
   register int err;
-  time_t boot;
+  intmax_t boot, pid;
   const char *user_name;
   const char *host_name;
   char *lock_info_str;
@@ -361,14 +354,16 @@ lock_file_1 (char *lfname, int force)
   else
     host_name = "";
   lock_info_str = (char *)alloca (strlen (user_name) + strlen (host_name)
-                                 + LOCK_PID_MAX + 30);
+                                 + 2 * INT_STRLEN_BOUND (intmax_t)
+                                 + sizeof "@.:");
+  pid = getpid ();
 
   if (boot)
-    sprintf (lock_info_str, "%s@%s.%lu:%lu", user_name, host_name,
-            (unsigned long) getpid (), (unsigned long) boot);
+    sprintf (lock_info_str, "%s@%s.%"PRIdMAX":%"PRIdMAX,
+            user_name, host_name, pid, boot);
   else
-    sprintf (lock_info_str, "%s@%s.%lu", user_name, host_name,
-            (unsigned long) getpid ());
+    sprintf (lock_info_str, "%s@%s.%"PRIdMAX,
+            user_name, host_name, pid);
 
   err = symlink (lock_info_str, lfname);
   if (errno == EEXIST && force)
@@ -397,8 +392,9 @@ static int
 current_lock_owner (lock_info_type *owner, char *lfname)
 {
   int ret;
-  size_t len;
-  int local_owner = 0;
+  ptrdiff_t len;
+  lock_info_type local_owner;
+  intmax_t n;
   char *at, *dot, *colon;
   char readlink_buf[READLINK_BUFSIZE];
   char *lfinfo = emacs_readlink (lfname, readlink_buf);
@@ -408,12 +404,9 @@ current_lock_owner (lock_info_type *owner, char *lfname)
     return errno == ENOENT ? 0 : -1;
 
   /* Even if the caller doesn't want the owner info, we still have to
-     read it to determine return value, so allocate it.  */
+     read it to determine return value.  */
   if (!owner)
-    {
-      owner = (lock_info_type *) alloca (sizeof (lock_info_type));
-      local_owner = 1;
-    }
+    owner = &local_owner;
 
   /* Parse USER@HOST.PID:BOOT_TIME.  If can't parse, return -1.  */
   /* The USER is everything before the last @.  */
@@ -427,24 +420,34 @@ current_lock_owner (lock_info_type *owner, char *lfname)
     }
   len = at - lfinfo;
   owner->user = (char *) xmalloc (len + 1);
-  strncpy (owner->user, lfinfo, len);
+  memcpy (owner->user, lfinfo, len);
   owner->user[len] = 0;
 
   /* The PID is everything from the last `.' to the `:'.  */
-  owner->pid = atoi (dot + 1);
-  colon = dot;
-  while (*colon && *colon != ':')
-    colon++;
+  errno = 0;
+  n = strtoimax (dot + 1, NULL, 10);
+  owner->pid =
+    ((0 <= n && n <= TYPE_MAXIMUM (pid_t)
+      && (TYPE_MAXIMUM (pid_t) < INTMAX_MAX || errno != ERANGE))
+     ? n : 0);
+
+  colon = strchr (dot + 1, ':');
   /* After the `:', if there is one, comes the boot time.  */
-  if (*colon == ':')
-    owner->boot_time = atoi (colon + 1);
-  else
-    owner->boot_time = 0;
+  n = 0;
+  if (colon)
+    {
+      errno = 0;
+      n = strtoimax (colon + 1, NULL, 10);
+    }
+  owner->boot_time =
+    ((0 <= n && n <= TYPE_MAXIMUM (time_t)
+      && (TYPE_MAXIMUM (time_t) < INTMAX_MAX || errno != ERANGE))
+     ? n : 0);
 
   /* The host is everything in between.  */
   len = dot - at - 1;
   owner->host = (char *) xmalloc (len + 1);
-  strncpy (owner->host, at + 1, len);
+  memcpy (owner->host, at + 1, len);
   owner->host[len] = 0;
 
   /* We're done looking at the link info.  */
@@ -476,7 +479,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
     }
 
   /* Avoid garbage.  */
-  if (local_owner || ret <= 0)
+  if (owner == &local_owner || ret <= 0)
     {
       FREE_LOCK_INFO (*owner);
     }
@@ -539,6 +542,7 @@ lock_file (Lisp_Object fn)
   register Lisp_Object attack, orig_fn, encoded_fn;
   register char *lfname, *locker;
   lock_info_type lock_info;
+  intmax_t pid;
   struct gcpro gcpro1;
 
   /* Don't do locking while dumping Emacs.
@@ -577,9 +581,10 @@ lock_file (Lisp_Object fn)
 
   /* Else consider breaking the lock */
   locker = (char *) alloca (strlen (lock_info.user) + strlen (lock_info.host)
-                           + LOCK_PID_MAX + 9);
-  sprintf (locker, "%s@%s (pid %lu)", lock_info.user, lock_info.host,
-           lock_info.pid);
+                           + INT_STRLEN_BOUND (intmax_t) + sizeof "@ (pid )");
+  pid = lock_info.pid;
+  sprintf (locker, "%s@%s (pid %"PRIdMAX")",
+          lock_info.user, lock_info.host, pid);
   FREE_LOCK_INFO (lock_info);
 
   attack = call2 (intern ("ask-user-about-lock"), fn, build_string (locker));