Fix crash in json{b}_populate_recordset() and json{b}_to_recordset().

As of commit 37a795a60, populate_recordset_worker() tried to pass back
(as rsi.setDesc) a tupdesc that it also had cached in its fn_extra.
But the core executor would free the passed-back tupdesc, risking a
crash if the function were called again in the same query.  The safest
and least invasive way to fix that is to make an extra tupdesc copy
to pass back.

While at it, I failed to resist the temptation to get rid of unnecessary
get_fn_expr_argtype() calls here and in populate_record_worker().

Per report from Dmitry Dolgov; thanks to Michael Paquier and
Andrew Gierth for investigation and discussion.

Discussion: https://postgr.es/m/CA+q6zcWzN9ztCfR47ZwgTr1KLnuO6BAY6FurxXhovP4hxr+yOQ@mail.gmail.com

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index e358b5ad1339c1de12438ab5d3fdd9a09fb12fb3..06f8d281685db2c6933d39bac47e6aec794980e2 100644 (file)
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -421,9 +421,9 @@ static void sn_scalar(void *state, char *token, JsonTokenType tokentype);
 
 /* worker functions for populate_record, to_record, populate_recordset and to_recordset */
 static Datum populate_recordset_worker(FunctionCallInfo fcinfo, const char *funcname,
-                                                 bool have_record_arg);
+                                                 bool is_json, bool have_record_arg);
 static Datum populate_record_worker(FunctionCallInfo fcinfo, const char *funcname,
-                                          bool have_record_arg);
+                                          bool is_json, bool have_record_arg);
 
 /* helper functions for populate_record[set] */
 static HeapTupleHeader populate_record(TupleDesc tupdesc, RecordIOData **record_p,
@@ -2296,25 +2296,29 @@ elements_scalar(void *state, char *token, JsonTokenType tokentype)
 Datum
 jsonb_populate_record(PG_FUNCTION_ARGS)
 {
-       return populate_record_worker(fcinfo, "jsonb_populate_record", true);
+       return populate_record_worker(fcinfo, "jsonb_populate_record",
+                                                                 false, true);
 }
 
 Datum
 jsonb_to_record(PG_FUNCTION_ARGS)
 {
-       return populate_record_worker(fcinfo, "jsonb_to_record", false);
+       return populate_record_worker(fcinfo, "jsonb_to_record",
+                                                                 false, false);
 }
 
 Datum
 json_populate_record(PG_FUNCTION_ARGS)
 {
-       return populate_record_worker(fcinfo, "json_populate_record", true);
+       return populate_record_worker(fcinfo, "json_populate_record",
+                                                                 true, true);
 }
 
 Datum
 json_to_record(PG_FUNCTION_ARGS)
 {
-       return populate_record_worker(fcinfo, "json_to_record", false);
+       return populate_record_worker(fcinfo, "json_to_record",
+                                                                 true, false);
 }
 
 /* helper function for diagnostics */
@@ -3203,12 +3207,15 @@ populate_record(TupleDesc tupdesc,
        return res->t_data;
 }
 
+/*
+ * common worker for json{b}_populate_record() and json{b}_to_record()
+ * is_json and have_record_arg identify the specific function
+ */
 static Datum
 populate_record_worker(FunctionCallInfo fcinfo, const char *funcname,
-                                          bool have_record_arg)
+                                          bool is_json, bool have_record_arg)
 {
        int                     json_arg_num = have_record_arg ? 1 : 0;
-       Oid                     jtype = get_fn_expr_argtype(fcinfo->flinfo, json_arg_num);
        JsValue         jsv = {0};
        HeapTupleHeader rec;
        Datum           rettuple;
@@ -3216,8 +3223,6 @@ populate_record_worker(FunctionCallInfo fcinfo, const char *funcname,
        MemoryContext fnmcxt = fcinfo->flinfo->fn_mcxt;
        PopulateRecordCache *cache = fcinfo->flinfo->fn_extra;
 
-       Assert(jtype == JSONOID || jtype == JSONBOID);
-
        /*
         * If first time through, identify input/result record type.  Note that
         * this stanza looks only at fcinfo context, which can't change during the
@@ -3303,9 +3308,9 @@ populate_record_worker(FunctionCallInfo fcinfo, const char *funcname,
                        PG_RETURN_NULL();
        }
 
-       jsv.is_json = jtype == JSONOID;
+       jsv.is_json = is_json;
 
-       if (jsv.is_json)
+       if (is_json)
        {
                text       *json = PG_GETARG_TEXT_PP(json_arg_num);
 
@@ -3489,25 +3494,29 @@ hash_scalar(void *state, char *token, JsonTokenType tokentype)
 Datum
 jsonb_populate_recordset(PG_FUNCTION_ARGS)
 {
-       return populate_recordset_worker(fcinfo, "jsonb_populate_recordset", true);
+       return populate_recordset_worker(fcinfo, "jsonb_populate_recordset",
+                                                                        false, true);
 }
 
 Datum
 jsonb_to_recordset(PG_FUNCTION_ARGS)
 {
-       return populate_recordset_worker(fcinfo, "jsonb_to_recordset", false);
+       return populate_recordset_worker(fcinfo, "jsonb_to_recordset",
+                                                                        false, false);
 }
 
 Datum
 json_populate_recordset(PG_FUNCTION_ARGS)
 {
-       return populate_recordset_worker(fcinfo, "json_populate_recordset", true);
+       return populate_recordset_worker(fcinfo, "json_populate_recordset",
+                                                                        true, true);
 }
 
 Datum
 json_to_recordset(PG_FUNCTION_ARGS)
 {
-       return populate_recordset_worker(fcinfo, "json_to_recordset", false);
+       return populate_recordset_worker(fcinfo, "json_to_recordset",
+                                                                        true, false);
 }
 
 static void
@@ -3544,14 +3553,14 @@ populate_recordset_record(PopulateRecordsetState *state, JsObject *obj)
 }
 
 /*
- * common worker for json_populate_recordset() and json_to_recordset()
+ * common worker for json{b}_populate_recordset() and json{b}_to_recordset()
+ * is_json and have_record_arg identify the specific function
  */
 static Datum
 populate_recordset_worker(FunctionCallInfo fcinfo, const char *funcname,
-                                                 bool have_record_arg)
+                                                 bool is_json, bool have_record_arg)
 {
        int                     json_arg_num = have_record_arg ? 1 : 0;
-       Oid                     jtype = get_fn_expr_argtype(fcinfo->flinfo, json_arg_num);
        ReturnSetInfo *rsi;
        MemoryContext old_cxt;
        HeapTupleHeader rec;
@@ -3662,7 +3671,7 @@ populate_recordset_worker(FunctionCallInfo fcinfo, const char *funcname,
        state->cache = cache;
        state->rec = rec;
 
-       if (jtype == JSONOID)
+       if (is_json)
        {
                text       *json = PG_GETARG_TEXT_PP(json_arg_num);
                JsonLexContext *lex;
@@ -3693,8 +3702,6 @@ populate_recordset_worker(FunctionCallInfo fcinfo, const char *funcname,
                bool            skipNested = false;
                JsonbIteratorToken r;
 
-               Assert(jtype == JSONBOID);
-
                if (JB_ROOT_IS_SCALAR(jb) || !JB_ROOT_IS_ARRAY(jb))
                        ereport(ERROR,
                                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -3726,8 +3733,13 @@ populate_recordset_worker(FunctionCallInfo fcinfo, const char *funcname,
                }
        }
 
+       /*
+        * Note: we must copy the cached tupdesc because the executor will free
+        * the passed-back setDesc, but we want to hang onto the cache in case
+        * we're called again in the same query.
+        */
        rsi->setResult = state->tuple_store;
-       rsi->setDesc = cache->c.io.composite.tupdesc;
+       rsi->setDesc = CreateTupleDescCopy(cache->c.io.composite.tupdesc);
 
        PG_RETURN_NULL();
 }

diff --git a/src/test/regress/expected/json.out b/src/test/regress/expected/json.out
index d514c621261f72f65ccb842ac44e1a2b09f4bd1f..6020feeea41bfd224e486357eb1e4a65344d4b69 100644 (file)
--- a/src/test/regress/expected/json.out
+++ b/src/test/regress/expected/json.out
@@ -1841,6 +1841,16 @@ SELECT json_populate_recordset(row(1,2), '[{"f1": 0, "f2": 1}]');
  (0,1)
 (1 row)
 
+SELECT i, json_populate_recordset(row(i,50), '[{"f1":"42"},{"f2":"43"}]')
+FROM (VALUES (1),(2)) v(i);
+ i | json_populate_recordset 
+---+-------------------------
+ 1 | (42,50)
+ 1 | (1,43)
+ 2 | (42,50)
+ 2 | (2,43)
+(4 rows)
+
 -- composite domain
 SELECT json_populate_recordset(null::j_ordered_pair, '[{"x": 0, "y": 1}]');
  json_populate_recordset 

diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index e5c2577dc2fb364fb051df2718e006f028e6b4e6..f045e0853843996b944e482bae4c3603272d45d0 100644 (file)
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -2523,6 +2523,16 @@ SELECT jsonb_populate_recordset(row(1,2), '[{"f1": 0, "f2": 1}]');
  (0,1)
 (1 row)
 
+SELECT i, jsonb_populate_recordset(row(i,50), '[{"f1":"42"},{"f2":"43"}]')
+FROM (VALUES (1),(2)) v(i);
+ i | jsonb_populate_recordset 
+---+--------------------------
+ 1 | (42,50)
+ 1 | (1,43)
+ 2 | (42,50)
+ 2 | (2,43)
+(4 rows)
+
 -- composite domain
 SELECT jsonb_populate_recordset(null::jb_ordered_pair, '[{"x": 0, "y": 1}]');
  jsonb_populate_recordset 

diff --git a/src/test/regress/sql/json.sql b/src/test/regress/sql/json.sql
index 4e65bb0d44485e07b0432e7a6875c341949ffcc2..97c75420e9c02808ff92f47d38b1227796e6bf09 100644 (file)
--- a/src/test/regress/sql/json.sql
+++ b/src/test/regress/sql/json.sql
@@ -547,6 +547,8 @@ select * from json_populate_recordset(row('def',99,null)::jpop,'[{"a":[100,200,3
 -- anonymous record type
 SELECT json_populate_recordset(null::record, '[{"x": 0, "y": 1}]');
 SELECT json_populate_recordset(row(1,2), '[{"f1": 0, "f2": 1}]');
+SELECT i, json_populate_recordset(row(i,50), '[{"f1":"42"},{"f2":"43"}]')
+FROM (VALUES (1),(2)) v(i);
 
 -- composite domain
 SELECT json_populate_recordset(null::j_ordered_pair, '[{"x": 0, "y": 1}]');

diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index d0ab6026ec589a7b285bf6cca78d197752dc230d..bd82fd13f7db8034ad1cb19b7402c6125899fe14 100644 (file)
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -663,6 +663,8 @@ SELECT * FROM jsonb_populate_recordset(row('def',99,NULL)::jbpop,'[{"a":[100,200
 -- anonymous record type
 SELECT jsonb_populate_recordset(null::record, '[{"x": 0, "y": 1}]');
 SELECT jsonb_populate_recordset(row(1,2), '[{"f1": 0, "f2": 1}]');
+SELECT i, jsonb_populate_recordset(row(i,50), '[{"f1":"42"},{"f2":"43"}]')
+FROM (VALUES (1),(2)) v(i);
 
 -- composite domain
 SELECT jsonb_populate_recordset(null::jb_ordered_pair, '[{"x": 0, "y": 1}]');