Fix jsonb_object_agg crash after eliminating null-valued pairs.

In commit b61aa76e4 I added an assumption in jsonb_object_agg_finalfn
that it'd be okay to apply uniqueifyJsonbObject repeatedly to a
JsonbValue.  I should have studied that code more closely first,
because in skip_nulls mode it removed leading nulls by changing the
"pairs" array start pointer.  This broke the data structure's
invariants in two ways: pairs no longer references a repalloc-able
chunk, and the distance from pairs to the end of its array is less
than parseState->size.  So any subsequent addition of more pairs is
at high risk of clobbering memory and/or causing repalloc to crash.
Unfortunately, adding more pairs is exactly what will happen when the
aggregate is being used as a window function.

Fix by rewriting uniqueifyJsonbObject to not do that.  The prior
coding had little to recommend it anyway.

Reported-by: Alexander Lakhin <[email protected]>
Author: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/ec5e96fb-ee49-4e5f-8a09-3f72b4780538@gmail.com

diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index c4fe6e00dcd2ba25f0b8b598028d8944cc504a78..dcf84c3fddc234ac8198d4bc02362d47dc91536b 100644 (file)
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -1755,8 +1755,11 @@ jsonb_object_agg_finalfn(PG_FUNCTION_ARGS)
     * the stored JsonbValue data structure.  Fortunately, the WJB_END_OBJECT
     * action will only destructively change fields in the JsonbInState struct
     * itself, so we can simply invoke pushJsonbValue on a local copy of that.
-    * (This technique results in running uniqueifyJsonbObject each time, but
-    * for now we won't bother trying to avoid that.)
+    * Note that this will run uniqueifyJsonbObject each time; that's hard to
+    * avoid, since duplicate pairs may have been added since the previous
+    * finalization.  We assume uniqueifyJsonbObject can be applied repeatedly
+    * (with the same unique_keys/skip_nulls options) without damaging the
+    * data structure.
     */
    result = arg->pstate;
 

diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 28e1ee18ce1c0a7e9554ebb9199f700db58ade17..5911b7c4d5705bba08cc3db37e3505d6a1714d93 100644 (file)
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -2069,12 +2069,14 @@ lengthCompareJsonbPair(const void *a, const void *b, void *binequal)
 static void
 uniqueifyJsonbObject(JsonbValue *object, bool unique_keys, bool skip_nulls)
 {
+   JsonbPair  *pairs = object->val.object.pairs;
+   int         nPairs = object->val.object.nPairs;
    bool        hasNonUniq = false;
 
    Assert(object->type == jbvObject);
 
-   if (object->val.object.nPairs > 1)
-       qsort_arg(object->val.object.pairs, object->val.object.nPairs, sizeof(JsonbPair),
+   if (nPairs > 1)
+       qsort_arg(pairs, nPairs, sizeof(JsonbPair),
                  lengthCompareJsonbPair, &hasNonUniq);
 
    if (hasNonUniq && unique_keys)
@@ -2084,36 +2086,25 @@ uniqueifyJsonbObject(JsonbValue *object, bool unique_keys, bool skip_nulls)
 
    if (hasNonUniq || skip_nulls)
    {
-       JsonbPair  *ptr,
-                  *res;
+       int         nNewPairs = 0;
 
-       while (skip_nulls && object->val.object.nPairs > 0 &&
-              object->val.object.pairs->value.type == jbvNull)
+       for (int i = 0; i < nPairs; i++)
        {
-           /* If skip_nulls is true, remove leading items with null */
-           object->val.object.pairs++;
-           object->val.object.nPairs--;
-       }
-
-       if (object->val.object.nPairs > 0)
-       {
-           ptr = object->val.object.pairs + 1;
-           res = object->val.object.pairs;
-
-           while (ptr - object->val.object.pairs < object->val.object.nPairs)
-           {
-               /* Avoid copying over duplicate or null */
-               if (lengthCompareJsonbStringValue(ptr, res) != 0 &&
-                   (!skip_nulls || ptr->value.type != jbvNull))
-               {
-                   res++;
-                   if (ptr != res)
-                       memcpy(res, ptr, sizeof(JsonbPair));
-               }
-               ptr++;
-           }
+           JsonbPair  *ptr = pairs + i;
 
-           object->val.object.nPairs = res + 1 - object->val.object.pairs;
+           /* Skip duplicate keys */
+           if (nNewPairs > 0 &&
+               lengthCompareJsonbStringValue(&pairs[nNewPairs - 1].key,
+                                             &ptr->key) == 0)
+               continue;
+           /* Skip null values, if told to */
+           if (skip_nulls && ptr->value.type == jbvNull)
+               continue;
+           /* Emit this pair, but avoid no-op copy */
+           if (i > nNewPairs)
+               pairs[nNewPairs] = *ptr;
+           nNewPairs++;
        }
+       object->val.object.nPairs = nNewPairs;
    }
 }

diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 5a1eb18aba29ec6e5eacfe7ace3ede46904b2b4b..2ab7a0df0752eace7b19afaf2c8898f8cd77dc57 100644 (file)
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -1582,6 +1582,38 @@ SELECT jsonb_object_agg(1, NULL::jsonb);
 
 SELECT jsonb_object_agg(NULL, '{"a":1}');
 ERROR:  field name must not be null
+SELECT jsonb_object_agg_unique(i, null) OVER (ORDER BY i)
+  FROM generate_series(1, 10) g(i);
+                                             jsonb_object_agg_unique                                             
+-----------------------------------------------------------------------------------------------------------------
+ {"1": null}
+ {"1": null, "2": null}
+ {"1": null, "2": null, "3": null}
+ {"1": null, "2": null, "3": null, "4": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null, "6": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null, "6": null, "7": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null, "6": null, "7": null, "8": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null, "6": null, "7": null, "8": null, "9": null}
+ {"1": null, "2": null, "3": null, "4": null, "5": null, "6": null, "7": null, "8": null, "9": null, "10": null}
+(10 rows)
+
+SELECT jsonb_object_agg_unique_strict(i, null) OVER (ORDER BY i)
+  FROM generate_series(1, 10) g(i);
+ jsonb_object_agg_unique_strict 
+--------------------------------
+ {}
+ {}
+ {}
+ {}
+ {}
+ {}
+ {}
+ {}
+ {}
+ {}
+(10 rows)
+
 CREATE TEMP TABLE foo (serial_num int, name text, type text);
 INSERT INTO foo VALUES (847001,'t15','GE1043');
 INSERT INTO foo VALUES (847002,'t16','GE1043');

diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 57c11acddfee62503d9dd271b7b1f137c4a8380f..1453f50ee998ccd6da7c58fb9e23fead7fafbe39 100644 (file)
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -402,6 +402,12 @@ SELECT jsonb_build_object('{1,2,3}'::int[], 3);
 SELECT jsonb_object_agg(1, NULL::jsonb);
 SELECT jsonb_object_agg(NULL, '{"a":1}');
 
+SELECT jsonb_object_agg_unique(i, null) OVER (ORDER BY i)
+  FROM generate_series(1, 10) g(i);
+
+SELECT jsonb_object_agg_unique_strict(i, null) OVER (ORDER BY i)
+  FROM generate_series(1, 10) g(i);
+
 CREATE TEMP TABLE foo (serial_num int, name text, type text);
 INSERT INTO foo VALUES (847001,'t15','GE1043');
 INSERT INTO foo VALUES (847002,'t16','GE1043');