Correctly detect missing key in a key-value pair (#167)

* Correctly detect missing key in a key-value pair

The following invalid JSON document would be classified as valid JSON
without this change:
```
{
    "key1":"val1",
    {
        "key2":"val2"
    }
}
```

The issue was reported here - #165.

Signed-off-by: Gaurav Aggarwal <[email protected]>

* Update complexity threshold to 12

Signed-off-by: Gaurav Aggarwal <[email protected]>

* Fix memory estimates and formatting checks

Signed-off-by: Gaurav Aggarwal <[email protected]>

* Fix CBMC proofs

Signed-off-by: Gaurav Aggarwal <[email protected]>

---------

Signed-off-by: Gaurav Aggarwal <[email protected]>

Author: aggarg

.github/workflows/ci.yml

@@ -44,6 +44,7 @@ jobs:
         uses: FreeRTOS/CI-CD-Github-Actions/complexity@main
         with:
           path: ./
+          horrid_threshold: 12
 
   doxygen:
     runs-on: ubuntu-latest

docs/doxygen/include/size_table.md

@@ -9,12 +9,12 @@
     </tr>
     <tr>
         <td>core_json.c</td>
-        <td><center>2.9K</center></td>
-        <td><center>2.4K</center></td>
+        <td><center>3.1K</center></td>
+        <td><center>2.5K</center></td>
     </tr>
     <tr>
         <td><b>Total estimates</b></td>
-        <td><b><center>2.9K</center></b></td>
-        <td><b><center>2.4K</center></b></td>
+        <td><b><center>3.1K</center></b></td>
+        <td><b><center>2.5K</center></b></td>
     </tr>
 </table>

loop_invariants.patch

@@ -123,7 +123,7 @@ index 901b2e1..8bdd89c 100644
      {
          if( skipAnyScalar( buf, &i, max ) != true )
          {
-@@ -986,6 +1037,13 @@ static void skipObjectScalars( const char * buf,
+@@ -986,6 +1037,13 @@ static bool skipObjectScalars( const char * buf,
      i = *start;
 
      while( i < max )

source/core_json.c

@@ -971,14 +971,18 @@ static void skipArrayScalars( const char * buf,
  * @param[in,out] start  The index at which to begin.
  * @param[in] max  The size of the buffer.
  *
+ * @return true if a valid scalar key-value pairs were present;
+ * false otherwise.
+ *
  * @note Stops advance if a value is an object or array.
  */
-static void skipObjectScalars( const char * buf,
+static bool skipObjectScalars( const char * buf,
                                size_t * start,
                                size_t max )
 {
     size_t i = 0U;
     bool comma = false;
+    bool ret = true;
 
     coreJSON_ASSERT( ( buf != NULL ) && ( start != NULL ) && ( max > 0U ) );
 
@@ -988,13 +992,15 @@ static void skipObjectScalars( const char * buf,
     {
         if( skipString( buf, &i, max ) != true )
         {
+            ret = false;
             break;
         }
 
         skipSpace( buf, &i, max );
 
         if( ( i < max ) && ( buf[ i ] != ':' ) )
         {
+            ret = false;
             break;
         }
 
@@ -1009,6 +1015,7 @@ static void skipObjectScalars( const char * buf,
 
         if( skipAnyScalar( buf, &i, max ) != true )
         {
+            ret = false;
             break;
         }
 
@@ -1020,6 +1027,8 @@ static void skipObjectScalars( const char * buf,
             break;
         }
     }
+
+    return ret;
 }
 
 /**
@@ -1029,13 +1038,17 @@ static void skipObjectScalars( const char * buf,
  * @param[in,out] start  The index at which to begin.
  * @param[in] max  The size of the buffer.
  * @param[in] mode  The first character of an array '[' or object '{'.
+ *
+ * @return true if a valid scalers were present;
+ * false otherwise.
  */
-static void skipScalars( const char * buf,
+static bool skipScalars( const char * buf,
                          size_t * start,
                          size_t max,
                          char mode )
 {
     bool modeIsOpenBracket = ( bool ) isOpenBracket_( mode );
+    bool ret = true;
 
     /* assert function may be implemented in macro using a # or ## operator.
      * Using a local variable here to prevent macro replacement is subjected
@@ -1053,8 +1066,10 @@ static void skipScalars( const char * buf,
     }
     else
     {
-        skipObjectScalars( buf, start, max );
+        ret = skipObjectScalars( buf, start, max );
     }
+
+    return ret;
 }
 
 /**
@@ -1106,7 +1121,12 @@ static JSONStatus_t skipCollection( const char * buf,
                 }
 
                 stack[ depth ] = c;
-                skipScalars( buf, &i, max, stack[ depth ] );
+
+                if( skipScalars( buf, &i, max, stack[ depth ] ) != true )
+                {
+                    ret = JSONIllegalDocument;
+                }
+
                 break;
 
             case '}':
@@ -1120,7 +1140,10 @@ static JSONStatus_t skipCollection( const char * buf,
                     if( ( skipSpaceAndComma( buf, &i, max ) == true ) &&
                         isOpenBracket_( stack[ depth ] ) )
                     {
-                        skipScalars( buf, &i, max, stack[ depth ] );
+                        if( skipScalars( buf, &i, max, stack[ depth ] ) != true )
+                        {
+                            ret = JSONIllegalDocument;
+                        }
                     }
 
                     break;

test/cbmc/include/core_json_contracts.h

@@ -261,7 +261,7 @@ assigns( *start )
 ensures( skipCollectionPostconditions( result, buf, start, old( *start ), max ) )
 ;
 
-void skipScalars( const char * buf,
+bool skipScalars( const char * buf,
                   size_t * start,
                   size_t max,
                   char mode )
@@ -270,7 +270,7 @@ assigns( *start )
 ensures( isValidStart( *start, old( *start ), max ) )
 ;
 
-void skipObjectScalars( const char * buf,
+bool skipObjectScalars( const char * buf,
                         size_t * start,
                         size_t max )
 requires( isValidBufferWithStartIndex( buf, max, start ) )

test/unit-test/core_json_utest.c

@@ -168,6 +168,15 @@
 #define MISSING_VALUE_AFTER_KEY                            "{\"foo\":{\"bar\":}}"
 #define MISSING_VALUE_AFTER_KEY_LENGTH                     ( sizeof( MISSING_VALUE_AFTER_KEY ) - 1 )
 
+#define MISSING_KEY                                        "{\"foo\":\"bar\", {\"abc\":\"xyz\"}}"
+#define MISSING_KEY_LENGTH                                 ( sizeof( MISSING_KEY ) - 1 )
+
+#define MISSING_VALUE                                      "{\"foo\":{\"abc\":\"xyz\"}, \"bar\":}"
+#define MISSING_VALUE_LENGTH                               ( sizeof( MISSING_VALUE ) - 1 )
+
+#define MISSING_SEPERATOR                                  "{\"foo\" \"bar\"}"
+#define MISSING_SEPERATOR_LENGTH                           ( sizeof( MISSING_SEPERATOR ) - 1 )
+
 #define MISMATCHED_BRACKETS                                "{\"foo\":{\"bar\":\"xyz\"]}"
 #define MISMATCHED_BRACKETS_LENGTH                         ( sizeof( MISMATCHED_BRACKETS ) - 1 )
 
@@ -619,6 +628,18 @@ void test_JSON_Validate_Illegal_Documents( void )
                                 MISSING_VALUE_AFTER_KEY_LENGTH );
     TEST_ASSERT_EQUAL( JSONIllegalDocument, jsonStatus );
 
+    jsonStatus = JSON_Validate( MISSING_KEY,
+                                MISSING_KEY_LENGTH );
+    TEST_ASSERT_EQUAL( JSONIllegalDocument, jsonStatus );
+
+    jsonStatus = JSON_Validate( MISSING_VALUE,
+                                MISSING_VALUE_LENGTH );
+    TEST_ASSERT_EQUAL( JSONIllegalDocument, jsonStatus );
+
+    jsonStatus = JSON_Validate( MISSING_SEPERATOR,
+                                MISSING_SEPERATOR_LENGTH );
+    TEST_ASSERT_EQUAL( JSONIllegalDocument, jsonStatus );
+
     jsonStatus = JSON_Validate( MISMATCHED_BRACKETS,
                                 MISMATCHED_BRACKETS_LENGTH );
     TEST_ASSERT_EQUAL( JSONIllegalDocument, jsonStatus );