[Emacs-diffs] trunk r116068: update-game-score fixes for -m and integer overflow

[Paul Eggert]

------------------------------------------------------------
revno: 116068
revision-id: address@hidden
parent: address@hidden
fixes bug: http://debbugs.gnu.org/16428
committer: Paul Eggert <address@hidden>
branch nick: trunk
timestamp: Sun 2014-01-19 00:50:53 -0800
message:
  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.
modified:
  lib-src/ChangeLog              changelog-20091113204419-o5vbwnq5f7feedwu-1608
  lib-src/update-game-score.c    
updategamescore.c-20091113204419-o5vbwnq5f7feedwu-2389

=== modified file 'lib-src/ChangeLog'
--- a/lib-src/ChangeLog 2014-01-16 08:34:43 +0000
+++ b/lib-src/ChangeLog 2014-01-19 08:50:53 +0000
@@ -1,3 +1,36 @@
+2014-01-19  Paul Eggert  <address@hidden>
+
+       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  <address@hidden>
 
        Spelling fixes.

=== modified file 'lib-src/update-game-score.c'
--- a/lib-src/update-game-score.c       2014-01-01 07:43:34 +0000
+++ b/lib-src/update-game-score.c       2014-01-19 08:50:53 +0000
@@ -35,7 +35,9 @@
 
 #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 @@
 #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 @@
 
 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 @@
 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 @@
 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 @@
        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 @@
   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 @@
   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 @@
     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 @@
   }
 #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 @@
   }
 #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 @@
       {
        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 @@
 }
 
 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 @@
 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 @@
   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 @@
 unlock_file (const char *filename, void *state)
 {
   char *lockpath = (char *) state;
+  int saved_errno = errno;
   int ret = unlink (lockpath);
-  int saved_errno = errno;
+  int unlink_errno = errno;
   free (lockpath);
-  errno = saved_errno;
+  errno = ret < 0 ? unlink_errno : saved_errno;
   return ret;
 }