bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

fix -fsanitize=address findings


From: Bruno Haible
Subject: fix -fsanitize=address findings
Date: Sat, 09 Mar 2019 23:31:33 +0100
User-agent: KMail/5.1.3 (Linux/4.4.0-141-generic; KDE/5.18.0; x86_64; ; )

Applying CC="gcc -fsanitize=address" to a gnulib testdir, I see
(aside from side memory leak reports):

ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7ffc5711adc0 at pc 
0x000000402bb9 bp 0x7ffc5711ab80 sp 0x7ffc5711ab78
READ of size 8 at 0x7ffc5711adc0 thread T0
    #0 0x402bb8 in rpl_strfmon_l ../../gllib/strfmon_l.c:50
    #1 0x401899 in main ../../gltests/test-strfmon_l.c:47

There is already a comment
  /* Hack: Consume more arguments than those that are actually given.  */
in the code. Time to remove this hack, and replace it with portable code.


2019-03-09  Bruno Haible  <address@hidden>

        strfmon_l: Fix -fsanitize=address finding.
        * lib/strfmon_l.c: Include <errno.h>, <stdbool.h>, <stdlib.h>,
        <string.h>.
        (MAX_ARGS): Renamed from MAX_ARG_WORDS.
        (directive_t, directives_t): New types.
        (fmon_parse): New function.
        (rpl_strfmon_l): Don't call va_arg more often than needed for the
        format string. Consume 'long double' arguments in places where the
        format string indicates so.
        * modules/strfmon_l (Depends-on): Add 'stdbool'.

diff --git a/lib/strfmon_l.c b/lib/strfmon_l.c
index b388c88..7f1d28f 100644
--- a/lib/strfmon_l.c
+++ b/lib/strfmon_l.c
@@ -19,13 +19,103 @@
 /* Specification.  */
 #include <monetary.h>
 
+#include <errno.h>
 #include <locale.h>
 #include <stdarg.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
 
 #undef strfmon_l
 
 /* This override can only support a limited number of arguments.  */
-#define MAX_ARG_WORDS 16
+#define MAX_ARGS 16
+
+/* A parsed directive.  */
+typedef struct
+{
+  bool needs_long_double;
+  const char *conversion_ptr;
+}
+directive_t;
+
+/* A parsed format string.  */
+typedef struct
+{
+  size_t count;
+  directive_t dir[MAX_ARGS];
+}
+directives_t;
+
+/* Parses a monetary format string.
+   Returns 0 and fills *DIRECTIVESP if valid.
+   Returns -1 if invalid.  */
+static int
+fmon_parse (const char *format, directives_t *directivesp)
+{
+  size_t count = 0;
+  const char *cp = format;
+
+  while (*cp != '\0')
+    {
+      if (*cp++ == '%')
+        {
+          /* Parse flags.  */
+          while (*cp == '=' || *cp == '^' || *cp == '+' || *cp == '('
+                 || *cp == '!' || *cp == '-')
+            {
+              if (*cp == '=')
+                {
+                  cp++;
+                  if (*cp == '\0')
+                    return -1;
+                }
+              cp++;
+            }
+          /* Parse field width.  */
+          while (*cp >= '0' && *cp <= '9')
+            cp++;
+          /* Parse left precision.  */
+          if (*cp == '#')
+            {
+              cp++;
+              while (*cp >= '0' && *cp <= '9')
+                cp++;
+            }
+          /* Parse right precision.  */
+          if (*cp == '.')
+            {
+              cp++;
+              while (*cp >= '0' && *cp <= '9')
+                cp++;
+            }
+          /* Now comes the conversion specifier.  */
+          if (*cp != '%')
+            {
+              if (count == MAX_ARGS)
+                /* Too many arguments.  */
+                return -1;
+
+              /* glibc supports an 'L' modifier before the conversion 
specifier.  */
+              if (*cp == 'L')
+                {
+                  cp++;
+                  directivesp->dir[count].needs_long_double = true;
+                }
+              else
+                directivesp->dir[count].needs_long_double = false;
+              if (!(*cp == 'i' || *cp == 'n'))
+                return -1;
+              directivesp->dir[count].conversion_ptr = cp;
+              count++;
+            }
+          cp++;
+        }
+    }
+
+  directivesp->count = count;
+  return 0;
+}
 
 ssize_t
 rpl_strfmon_l (char *s, size_t maxsize, locale_t locale, const char *format, 
...)
@@ -33,9 +123,8 @@ rpl_strfmon_l (char *s, size_t maxsize, locale_t locale, 
const char *format, ...
   /* Work around glibc 2.23 bug
      <https://sourceware.org/bugzilla/show_bug.cgi?id=19633>.  */
   va_list argptr;
-  double args[MAX_ARG_WORDS];
-  int i;
   locale_t orig_locale;
+  directives_t directives;
   ssize_t result;
 
   orig_locale = uselocale ((locale_t)0);
@@ -44,17 +133,101 @@ rpl_strfmon_l (char *s, size_t maxsize, locale_t locale, 
const char *format, ...
     /* errno is set.  */
     return -1;
 
-  va_start (argptr, format);
-  /* Hack: Consume more arguments than those that are actually given.  */
-  for (i = 0; i < MAX_ARG_WORDS; i++)
-    args[i] = va_arg (argptr, double);
-
-  result = strfmon_l (s, maxsize, locale, format,
-                      args[0], args[1], args[2], args[3], args[4], args[5],
-                      args[6], args[7], args[8], args[9], args[10], args[11],
-                      args[12], args[13], args[14], args[15]);
-
-  va_end (argptr);
+  /* The format string may consume 'double' or 'long double' arguments.
+     In order not to have to link with libffcall or libffi, convert all
+     arguments to 'long double', and use a modified format string that
+     requests 'long double' arguments.  But since 'long double' arguments
+     are only supported on glibc, do so only if the original format string
+     consumes at least one 'long double' argument.  */
+  if (fmon_parse (format, &directives) < 0)
+    {
+      errno = EINVAL;
+      result = -1;
+    }
+  else
+    {
+      bool use_long_double;
+      unsigned int i;
+
+      use_long_double = false;
+      for (i = 0; i < directives.count; i++)
+        if (directives.dir[i].needs_long_double)
+          {
+            use_long_double = true;
+            break;
+          }
+
+      va_start (argptr, format);
+
+      if (use_long_double)
+        {
+          char *ld_format;
+
+          /* Allocate room for the modified format string.  */
+          ld_format = (char *) malloc (strlen (format) + directives.count + 1);
+          if (ld_format == NULL)
+            {
+              errno = ENOMEM;
+              result = -1;
+            }
+          else
+            {
+              long double args[MAX_ARGS];
+
+              /* Create the modified format string.  */
+              {
+                const char *p = format;
+                char *dest = ld_format;
+                for (i = 0; i < directives.count; i++)
+                  {
+                    const char *q = directives.dir[i].conversion_ptr;
+                    memcpy (dest, p, q - p);
+                    dest += q - p;
+                    if (!directives.dir[i].needs_long_double)
+                      *dest++ = 'L';
+                    p = q;
+                  }
+                strcpy (dest, p);
+              }
+
+              /* Set up arguments array.  */
+              for (i = 0; i < directives.count; i++)
+                args[i] = (directives.dir[i].needs_long_double
+                           ? va_arg (argptr, long double)
+                           : (long double) va_arg (argptr, double));
+              /* Avoid uninitialized memory references.  */
+              for (; i < MAX_ARGS; i++)
+                args[i] = 0.0L;
+
+              result = strfmon_l (s, maxsize, locale, ld_format,
+                                  args[0], args[1], args[2], args[3], args[4],
+                                  args[5], args[6], args[7], args[8], args[9],
+                                  args[10], args[11], args[12], args[13],
+                                  args[14], args[15]);
+
+              free (ld_format);
+            }
+        }
+      else
+        {
+          double args[MAX_ARGS];
+
+          /* Set up arguments array.  */
+          for (i = 0; i < directives.count; i++)
+            args[i] = va_arg (argptr, double);
+          /* Avoid uninitialized memory references.  */
+          for (; i < MAX_ARGS; i++)
+            args[i] = 0.0;
+
+          result = strfmon_l (s, maxsize, locale, format,
+                              args[0], args[1], args[2], args[3], args[4],
+                              args[5], args[6], args[7], args[8], args[9],
+                              args[10], args[11], args[12], args[13], args[14],
+                              args[15]);
+        }
+
+      va_end (argptr);
+    }
 
   if (uselocale (orig_locale) == (locale_t)0)
     /* errno is set.  */
diff --git a/modules/strfmon_l b/modules/strfmon_l
index 42a4c96..16c339c 100644
--- a/modules/strfmon_l
+++ b/modules/strfmon_l
@@ -8,6 +8,7 @@ m4/strfmon_l.m4
 Depends-on:
 monetary
 extensions
+stdbool         [test $REPLACE_STRFMON_L = 1]
 
 configure.ac:
 gl_FUNC_STRFMON_L




reply via email to

[Prev in Thread] Current Thread [Next in Thread]