From 9a5e8ed283e583782b3f40e6ededcb005d5c0333 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 13 Jul 2018 14:16:47 -0400 Subject: [PATCH] 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 --- src/backend/utils/adt/jsonfuncs.c | 58 +++++++++++++++++------------ src/test/regress/expected/json.out | 10 +++++ src/test/regress/expected/jsonb.out | 10 +++++ src/test/regress/sql/json.sql | 2 + src/test/regress/sql/jsonb.sql | 2 + 5 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index e358b5ad13..06f8d28168 100644 --- 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 d514c62126..6020feeea4 100644 --- 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 e5c2577dc2..f045e08538 100644 --- 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 4e65bb0d44..97c75420e9 100644 --- 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 d0ab6026ec..bd82fd13f7 100644 --- 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}]');