update-game-score fixes for -m and integer overflow

* update-game-score.c: Include inttypes.h, stdbool.h.
(min): New macro, if not already defined.
(MAX_SCORES, main): Limit the maximum number of scores only from
limits imposed by the underyling platform, instead of the
arbitrary value 200.
(struct score_entry, main, read_score, write_score):
Scores are now intmax_t, not long.
(get_user_id): Reject user names containing spaces or newlines,
as they would mess up the score file.
Allow uids that don't fit in 'long'.
Increase the size of the buffer, to avoid overrun in weird cases.
(get_prefix, main): Use bool for boolean.
(main): Rewrite expr to avoid possibility of signed integer
overflow.  Don't allow newlines in data, as this would mess up
the score file.  Check for memory allocation failure when adding
the new score, or when unlockint the file.  Implement -m.
(read_score): Check for integer overflow when reading a score.
(read_score) [!HAVE_GETDELIM]: Check for integer overflow when
data gets very long.  Check only for space to delimit names,
since that's what's done in the HAVE_GETDELIM case.
(read_scores): New parameter ALLOC.  Change counts to ptrdiff_t.
All uses changed.  Use push_score to add individual scores;
that's simpler than repeating its contents.
(score_compare_reverse): Simplify.
(push_score): New parameter SIZE.  Change counts to ptrdiff_t.
All uses changed.  Check for integer overflow of size calculation.
(sort_scores, write_scores): Change counts to ptrdiff_t.
(unlock_file): Preserve errno on success, so that storage
exhaustion is diagnosed correctly.

Fixes: debbugs:16428

diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index dc4ec91c51267ea7c5758a00fd0c8d0bcf9f5652..50273e2a60ade40761b5092182e4f687f0bcfe8b 100644 (file)
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,36 @@
+2014-01-19  Paul Eggert  <eggert@cs.ucla.edu>
+
+       update-game-score fixes for -m and integer overflow (Bug#16428)
+       * update-game-score.c: Include inttypes.h, stdbool.h.
+       (min): New macro, if not already defined.
+       (MAX_SCORES, main): Limit the maximum number of scores only from
+       limits imposed by the underyling platform, instead of the
+       arbitrary value 200.
+       (struct score_entry, main, read_score, write_score):
+       Scores are now intmax_t, not long.
+       (get_user_id): Reject user names containing spaces or newlines,
+       as they would mess up the score file.
+       Allow uids that don't fit in 'long'.
+       Increase the size of the buffer, to avoid overrun in weird cases.
+       (get_prefix, main): Use bool for boolean.
+       (main): Rewrite expr to avoid possibility of signed integer
+       overflow.  Don't allow newlines in data, as this would mess up
+       the score file.  Check for memory allocation failure when adding
+       the new score, or when unlockint the file.  Implement -m.
+       (read_score): Check for integer overflow when reading a score.
+       (read_score) [!HAVE_GETDELIM]: Check for integer overflow when
+       data gets very long.  Check only for space to delimit names,
+       since that's what's done in the HAVE_GETDELIM case.
+       (read_scores): New parameter ALLOC.  Change counts to ptrdiff_t.
+       All uses changed.  Use push_score to add individual scores;
+       that's simpler than repeating its contents.
+       (score_compare_reverse): Simplify.
+       (push_score): New parameter SIZE.  Change counts to ptrdiff_t.
+       All uses changed.  Check for integer overflow of size calculation.
+       (sort_scores, write_scores): Change counts to ptrdiff_t.
+       (unlock_file): Preserve errno on success, so that storage
+       exhaustion is diagnosed correctly.
+
 2014-01-05  Paul Eggert  <eggert@cs.ucla.edu>
 
        Spelling fixes.

diff --git a/lib-src/update-game-score.c b/lib-src/update-game-score.c
index e2bf93bb1eb30603c754f3a1dabc6c0dcaae16f1..916a351432d7bbd5fae8c370cf49a2334a32953f 100644 (file)
--- a/lib-src/update-game-score.c
+++ b/lib-src/update-game-score.c
@@ -35,7 +35,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <unistd.h>
 #include <errno.h>
+#include <inttypes.h>
 #include <limits.h>
+#include <stdbool.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -50,8 +52,11 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "ntlib.h"
 #endif
 
+#ifndef min
+# define min(a,b) ((a) < (b) ? (a) : (b))
+#endif
+
 #define MAX_ATTEMPTS 5
-#define MAX_SCORES 200
 #define MAX_DATA_LEN 1024
 
 #ifndef HAVE_DIFFTIME
@@ -76,18 +81,21 @@ static int unlock_file (const char *filename, void *state);
 
 struct score_entry
 {
-  long score;
+  intmax_t score;
   char *username;
   char *data;
 };
 
+#define MAX_SCORES min (PTRDIFF_MAX, SIZE_MAX / sizeof (struct score_entry))
+
 static int read_scores (const char *filename, struct score_entry **scores,
-                       int *count);
-static int push_score (struct score_entry **scores, int *count,
-                      int newscore, char *username, char *newdata);
-static void sort_scores (struct score_entry *scores, int count, int reverse);
+                       ptrdiff_t *count, ptrdiff_t *alloc);
+static int push_score (struct score_entry **scores, ptrdiff_t *count,
+                      ptrdiff_t *size, struct score_entry const *newscore);
+static void sort_scores (struct score_entry *scores, ptrdiff_t count,
+                        bool reverse);
 static int write_scores (const char *filename,
-                        const struct score_entry *scores, int count);
+                        const struct score_entry *scores, ptrdiff_t count);
 
 static _Noreturn void
 lose (const char *msg)
@@ -107,19 +115,19 @@ static char *
 get_user_id (void)
 {
   struct passwd *buf = getpwuid (getuid ());
-  if (!buf)
+  if (!buf || strchr (buf->pw_name, ' ') || strchr (buf->pw_name, '\n'))
     {
-      long uid = getuid ();
-      char *name = malloc (sizeof uid * CHAR_BIT / 3 + 1);
+      intmax_t uid = getuid ();
+      char *name = malloc (sizeof uid * CHAR_BIT / 3 + 4);
       if (name)
-       sprintf (name, "%ld", uid);
+       sprintf (name, "%"PRIdMAX, uid);
       return name;
     }
   return buf->pw_name;
 }
 
 static const char *
-get_prefix (int running_suid, const char *user_prefix)
+get_prefix (bool running_suid, const char *user_prefix)
 {
   if (!running_suid && user_prefix == NULL)
     lose ("Not using a shared game directory, and no prefix given.");
@@ -137,14 +145,18 @@ get_prefix (int running_suid, const char *user_prefix)
 int
 main (int argc, char **argv)
 {
-  int c, running_suid;
+  int c;
+  bool running_suid;
   void *lockstate;
-  char *user_id, *scorefile;
+  char *scorefile;
+  char *nl;
   const char *prefix, *user_prefix = NULL;
   struct stat buf;
   struct score_entry *scores;
-  int newscore, scorecount, reverse = 0, max = MAX_SCORES;
-  char *newdata;
+  struct score_entry newscore;
+  bool reverse = false;
+  ptrdiff_t scorecount, scorealloc;
+  ptrdiff_t max_scores = MAX_SCORES;
 
   srand (time (0));
 
@@ -161,15 +173,18 @@ main (int argc, char **argv)
        reverse = 1;
        break;
       case 'm':
-       max = atoi (optarg);
-       if (max > MAX_SCORES)
-         max = MAX_SCORES;
+       {
+         intmax_t m = strtoimax (optarg, 0, 10);
+         if (m < 0)
+           usage (EXIT_FAILURE);
+         max_scores = min (m, MAX_SCORES);
+       }
        break;
       default:
        usage (EXIT_FAILURE);
       }
 
-  if (optind+3 != argc)
+  if (argc - optind != 3)
     usage (EXIT_FAILURE);
 
   running_suid = (getuid () != geteuid ());
@@ -183,13 +198,18 @@ main (int argc, char **argv)
   strcpy (scorefile, prefix);
   strcat (scorefile, "/");
   strcat (scorefile, argv[optind]);
-  newscore = atoi (argv[optind+1]);
-  newdata = argv[optind+2];
-  if (strlen (newdata) > MAX_DATA_LEN)
-    newdata[MAX_DATA_LEN] = '\0';
 
-  user_id = get_user_id ();
-  if (user_id == NULL)
+  newscore.score = strtoimax (argv[optind + 1], 0, 10);
+
+  newscore.data = argv[optind + 2];
+  if (strlen (newscore.data) > MAX_DATA_LEN)
+    newscore.data[MAX_DATA_LEN] = '\0';
+  nl = strchr (newscore.data, '\n');
+  if (nl)
+    *nl = '\0';
+
+  newscore.username = get_user_id ();
+  if (! newscore.username)
     lose_syserr ("Couldn't determine user id");
 
   if (stat (scorefile, &buf) < 0)
@@ -198,29 +218,34 @@ main (int argc, char **argv)
   if (lock_file (scorefile, &lockstate) < 0)
     lose_syserr ("Failed to lock scores file");
 
-  if (read_scores (scorefile, &scores, &scorecount) < 0)
+  if (read_scores (scorefile, &scores, &scorecount, &scorealloc) < 0)
     {
       unlock_file (scorefile, lockstate);
       lose_syserr ("Failed to read scores file");
     }
-  push_score (&scores, &scorecount, newscore, user_id, newdata);
+  if (push_score (&scores, &scorecount, &scorealloc, &newscore) < 0)
+    {
+      unlock_file (scorefile, lockstate);
+      lose_syserr ("Failed to add score");
+    }
   sort_scores (scores, scorecount, reverse);
   /* Limit the number of scores.  If we're using reverse sorting, then
      also increment the beginning of the array, to skip over the
      *smallest* scores.  Otherwise, just decrementing the number of
      scores suffices, since the smallest is at the end. */
-  if (scorecount > MAX_SCORES)
+  if (scorecount > max_scores)
     {
       if (reverse)
-       scores += (scorecount - MAX_SCORES);
-      scorecount = MAX_SCORES;
+       scores += scorecount - max_scores;
+      scorecount = max_scores;
     }
   if (write_scores (scorefile, scores, scorecount) < 0)
     {
       unlock_file (scorefile, lockstate);
       lose_syserr ("Failed to write scores file");
     }
-  unlock_file (scorefile, lockstate);
+  if (unlock_file (scorefile, lockstate) < 0)
+    lose_syserr ("Failed to unlock scores file");
   exit (EXIT_SUCCESS);
 }
 
@@ -234,8 +259,12 @@ read_score (FILE *f, struct score_entry *score)
     return 1;
   for (score->score = 0; (c = getc (f)) != EOF && isdigit (c); )
     {
+      if (INTMAX_MAX / 10 < score->score)
+       return -1;
       score->score *= 10;
-      score->score += (c-48);
+      if (INTMAX_MAX - (c - '0') < score->score)
+       return -1;
+      score->score += c - '0';
     }
   while ((c = getc (f)) != EOF
         && isspace (c))
@@ -254,18 +283,30 @@ read_score (FILE *f, struct score_entry *score)
   }
 #else
   {
-    int unameread = 0;
-    int unamelen = 30;
+    ptrdiff_t unameread = 0;
+    ptrdiff_t unamelen = 30;
     char *username = malloc (unamelen);
     if (!username)
       return -1;
 
-    while ((c = getc (f)) != EOF
-          && !isspace (c))
+    while ((c = getc (f)) != EOF && c != ' ')
       {
-       if (unameread >= unamelen-1)
-         if (!(username = realloc (username, unamelen *= 2)))
-           return -1;
+       if (unameread >= unamelen - 1)
+         {
+           ptrdiff_t unamelen_max = min (PTRDIFF_MAX, SIZE_MAX);
+           if (unamelen <= unamelen_max / 2)
+             unamelen *= 2;
+           else if (unamelen < unamelen_max)
+             unamelen = unamelen_max;
+           else
+             {
+               errno = ENOMEM;
+               return -1;
+             }
+           username = realloc (username, unamelen);
+           if (!username)
+             return -1;
+         }
        username[unameread] = c;
        unameread++;
       }
@@ -286,8 +327,8 @@ read_score (FILE *f, struct score_entry *score)
   }
 #else
   {
-    int cur = 0;
-    int len = 16;
+    ptrdiff_t cur = 0;
+    ptrdiff_t len = 16;
     char *buf = malloc (len);
     if (!buf)
       return -1;
@@ -296,6 +337,11 @@ read_score (FILE *f, struct score_entry *score)
       {
        if (cur >= len-1)
          {
+           if (min (PTRDIFF_MAX, SIZE_MAX) / 2 < len)
+             {
+               errno = ENOMEM;
+               return -1;
+             }
            if (!(buf = realloc (buf, len *= 2)))
              return -1;
          }
@@ -310,35 +356,25 @@ read_score (FILE *f, struct score_entry *score)
 }
 
 static int
-read_scores (const char *filename, struct score_entry **scores, int *count)
+read_scores (const char *filename, struct score_entry **scores,
+            ptrdiff_t *count, ptrdiff_t *alloc)
 {
-  int readval = -1, scorecount, cursize;
-  struct score_entry *ret;
+  int readval = -1;
+  ptrdiff_t scorecount = 0;
+  ptrdiff_t cursize = 0;
+  struct score_entry *ret = 0;
+  struct score_entry entry;
   FILE *f = fopen (filename, "r");
   int retval = -1;
   if (!f)
     return -1;
-  scorecount = 0;
-  cursize = 16;
-  ret = (struct score_entry *) malloc (sizeof (struct score_entry) * cursize);
-  if (ret)
-    {
-      while ((readval = read_score (f, &ret[scorecount])) == 0)
-       {
-         scorecount++;
-         if (scorecount >= cursize)
-           {
-             cursize *= 2;
-             ret = (struct score_entry *)
-               realloc (ret, (sizeof (struct score_entry) * cursize));
-             if (!ret)
-               break;
-           }
-       }
-    }
+  while ((readval = read_score (f, &entry)) == 0)
+    if (push_score (&ret, &scorecount, &cursize, &entry) < 0)
+      return -1;
   if (readval > 0)
     {
       *count = scorecount;
+      *alloc = cursize;
       *scores = ret;
       retval = 0;
     }
@@ -357,40 +393,53 @@ score_compare (const void *a, const void *b)
 static int
 score_compare_reverse (const void *a, const void *b)
 {
-  const struct score_entry *sa = (const struct score_entry *) a;
-  const struct score_entry *sb = (const struct score_entry *) b;
-  return (sa->score > sb->score) - (sa->score < sb->score);
+  return score_compare (b, a);
 }
 
 int
-push_score (struct score_entry **scores, int *count, int newscore, char *username, char *newdata)
+push_score (struct score_entry **scores, ptrdiff_t *count, ptrdiff_t *size,
+           struct score_entry const *newscore)
 {
- struct score_entry *newscores
-   = (struct score_entry *) realloc (*scores,
-                                    sizeof (struct score_entry) * ((*count) + 1));
-  if (!newscores)
-    return -1;
-  newscores[*count].score = newscore;
-  newscores[*count].username = username;
-  newscores[*count].data = newdata;
+  struct score_entry *newscores = *scores;
+  if (*count == *size)
+    {
+      ptrdiff_t newsize = *size;
+      if (newsize <= 0)
+       newsize = 1;
+      else if (newsize <= MAX_SCORES / 2)
+       newsize *= 2;
+      else if (newsize < MAX_SCORES)
+       newsize = MAX_SCORES;
+      else
+       {
+         errno = ENOMEM;
+         return -1;
+       }
+      newscores = realloc (newscores, sizeof *newscores * newsize);
+      if (!newscores)
+       return -1;
+      *scores = newscores;
+      *size = newsize;
+    }
+  newscores[*count] = *newscore;
   (*count) += 1;
-  *scores = newscores;
   return 0;
 }
 
 static void
-sort_scores (struct score_entry *scores, int count, int reverse)
+sort_scores (struct score_entry *scores, ptrdiff_t count, bool reverse)
 {
-  qsort (scores, count, sizeof (struct score_entry),
-       reverse ? score_compare_reverse : score_compare);
+  qsort (scores, count, sizeof *scores,
+        reverse ? score_compare_reverse : score_compare);
 }
 
 static int
-write_scores (const char *filename, const struct score_entry *scores, int count)
+write_scores (const char *filename, const struct score_entry *scores,
+             ptrdiff_t count)
 {
   int fd;
   FILE *f;
-  int i;
+  ptrdiff_t i;
   char *tempfile = malloc (strlen (filename) + strlen (".tempXXXXXX") + 1);
   if (!tempfile)
     return -1;
@@ -403,8 +452,9 @@ write_scores (const char *filename, const struct score_entry *scores, int count)
   if (! f)
     return -1;
   for (i = 0; i < count; i++)
-    if (fprintf (f, "%ld %s %s\n", scores[i].score, scores[i].username,
-               scores[i].data) < 0)
+    if (fprintf (f, "%"PRIdMAX" %s %s\n",
+                scores[i].score, scores[i].username, scores[i].data)
+       < 0)
       return -1;
   fclose (f);
   if (rename (tempfile, filename) < 0)
@@ -459,10 +509,11 @@ static int
 unlock_file (const char *filename, void *state)
 {
   char *lockpath = (char *) state;
-  int ret = unlink (lockpath);
   int saved_errno = errno;
+  int ret = unlink (lockpath);
+  int unlink_errno = errno;
   free (lockpath);
-  errno = saved_errno;
+  errno = ret < 0 ? unlink_errno : saved_errno;
   return ret;
 }