[glib: 1/2] gvariant: Fix bounds checking in GVariant text format parser

[Emmanuele Bassi <ebassi src gnome org>]

commit 10ee7301e8edb13e59143ee5653cd2b46e26c044
Author: Philip Withnall <withnall endlessm com>
Date:   Thu Aug 9 01:00:20 2018 +0100

    gvariant: Fix bounds checking in GVariant text format parser
    
    The token_stream_peek() functions were not doing any bounds checking, so
    could potentially read 1 byte off the end of the input blob. This was
    never noticed, since the input stream is almost always a nul-terminated
    string. However, g_variant_parse() does allow non-nul-terminated strings
    to be used with a @limit parameter, and the bugs become apparent under
    valgrind if that parameter is used.
    
    This includes modifications to the test cases to cover the
    non-nul-terminated case.
    
    Spotted by ossfuzz.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>

 glib/gvariant-parser.c | 21 ++++++++++++++-------
 glib/tests/gvariant.c  | 39 ++++++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 16 deletions(-)
---
diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c
index 3261bc1af..233a19f7c 100644
--- a/glib/gvariant-parser.c
+++ b/glib/gvariant-parser.c
@@ -260,6 +260,9 @@ token_stream_prepare (TokenStream *stream)
   stream->this = stream->stream;
   stream->stream = end;
 
+  /* We must have at least one byte in a token. */
+  g_assert (stream->stream - stream->this >= 1);
+
   return TRUE;
 }
 
@@ -276,7 +279,8 @@ token_stream_peek (TokenStream *stream,
   if (!token_stream_prepare (stream))
     return FALSE;
 
-  return stream->this[0] == first_char;
+  return stream->stream - stream->this >= 1 &&
+         stream->this[0] == first_char;
 }
 
 static gboolean
@@ -287,7 +291,8 @@ token_stream_peek2 (TokenStream *stream,
   if (!token_stream_prepare (stream))
     return FALSE;
 
-  return stream->this[0] == first_char &&
+  return stream->stream - stream->this >= 2 &&
+         stream->this[0] == first_char &&
          stream->this[1] == second_char;
 }
 
@@ -297,7 +302,8 @@ token_stream_is_keyword (TokenStream *stream)
   if (!token_stream_prepare (stream))
     return FALSE;
 
-  return g_ascii_isalpha (stream->this[0]) &&
+  return stream->stream - stream->this >= 2 &&
+         g_ascii_isalpha (stream->this[0]) &&
          g_ascii_isalpha (stream->this[1]);
 }
 
@@ -307,10 +313,11 @@ token_stream_is_numeric (TokenStream *stream)
   if (!token_stream_prepare (stream))
     return FALSE;
 
-  return (g_ascii_isdigit (stream->this[0]) ||
-          stream->this[0] == '-' ||
-          stream->this[0] == '+' ||
-          stream->this[0] == '.');
+  return (stream->stream - stream->this >= 1 &&
+          (g_ascii_isdigit (stream->this[0]) ||
+           stream->this[0] == '-' ||
+           stream->this[0] == '+' ||
+           stream->this[0] == '.'));
 }
 
 static gboolean
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index fdaed1acc..5aac3de53 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -3889,27 +3889,48 @@ test_parse_failures (void)
     "boolean 4",                "8-9:",            "can not parse as",
     "int32 true",               "6-10:",           "can not parse as",
     "[double 5, int32 5]",      "1-9,11-18:",      "common type",
-    "string 4",                 "7-8:",            "can not parse as"
+    "string 4",                 "7-8:",            "can not parse as",
+    "\x0a",                     "1:",              "expected value",
+    "((",                       "2:",              "expected value",
   };
   gint i;
 
   for (i = 0; i < G_N_ELEMENTS (test); i += 3)
     {
-      GError *error = NULL;
+      GError *error1 = NULL, *error2 = NULL;
       GVariant *value;
 
-      value = g_variant_parse (NULL, test[i], NULL, NULL, &error);
-      g_assert (value == NULL);
+      /* Copy the test string and drop its nul terminator, then use the @limit
+       * parameter of g_variant_parse() to set the length. This allows valgrind
+       * to catch 1-byte heap buffer overflows. */
+      gsize test_len = MAX (strlen (test[i]), 1);
+      gchar *test_blob = g_malloc0 (test_len);  /* no nul terminator */
 
-      if (!strstr (error->message, test[i+2]))
+      memcpy (test_blob, test[i], test_len);
+      value = g_variant_parse (NULL, test_blob, test_blob + test_len, NULL, &error1);
+      g_assert_null (value);
+
+      g_free (test_blob);
+
+      if (!strstr (error1->message, test[i+2]))
         g_error ("test %d: Can't find '%s' in '%s'", i / 3,
-                 test[i+2], error->message);
+                 test[i+2], error1->message);
 
-      if (!g_str_has_prefix (error->message, test[i+1]))
+      if (!g_str_has_prefix (error1->message, test[i+1]))
         g_error ("test %d: Expected location '%s' in '%s'", i / 3,
-                 test[i+1], error->message);
+                 test[i+1], error1->message);
+
+      /* Test again with the nul terminator this time. The behaviour should be
+       * the same. */
+      value = g_variant_parse (NULL, test[i], NULL, NULL, &error2);
+      g_assert_null (value);
+
+      g_assert_cmpint (error1->domain, ==, error2->domain);
+      g_assert_cmpint (error1->code, ==, error2->code);
+      g_assert_cmpstr (error1->message, ==, error2->message);
 
-      g_error_free (error);
+      g_clear_error (&error1);
+      g_clear_error (&error2);
     }
 }