From 670c0a1d474bf296dbcc1d6de912d4841f2ed643 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Jul 2020 15:19:46 -0400 Subject: [PATCH] Weaken type-OID-matching checks in array_recv and record_recv. Rather than always insisting on an exact match of the type OID in the data to the element type or column type we expect, complain only when both OIDs fall within the manually-assigned range. This acknowledges the reality that user-defined types don't have stable OIDs, while still preserving some of the mistake-detection value of the old test. (It's not entirely clear whether to error if one OID is manually assigned and the other isn't. But perhaps that case could arise in cross-version cases where a former extension type has been imported into core, so I let it pass.) This change allows us to remove the prohibition on binary transfer of user-defined arrays and composites in the recently-landed support for binary logical replication (commit 9de77b545). We can just unconditionally drop that check, since if the client has asked for binary transfer it must be >= v14 and must have this change. Discussion: https://postgr.es/m/CADK3HH+R3xMn=8t3Ct+uD+qJ1KD=Hbif5NFMJ+d5DkoCzp6Vgw@mail.gmail.com --- src/backend/replication/logical/proto.c | 17 ++------------- src/backend/utils/adt/arrayfuncs.c | 29 +++++++++++++++++++++---- src/backend/utils/adt/rowtypes.c | 28 ++++++++++++++++++++---- 3 files changed, 51 insertions(+), 23 deletions(-) diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c index 04b4f494bb9..9ff8097bf5f 100644 --- a/src/backend/replication/logical/proto.c +++ b/src/backend/replication/logical/proto.c @@ -494,22 +494,9 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar typclass = (Form_pg_type) GETSTRUCT(typtup); /* - * Choose whether to send in binary. Obviously, the option must be - * requested and the type must have a send function. Also, if the - * type is not built-in then it must not be a composite or array type. - * Such types contain type OIDs, which will likely not match at the - * receiver if it's not a built-in type. - * - * XXX this could be relaxed if we changed record_recv and array_recv - * to be less picky. - * - * XXX this fails to apply the restriction to domains over such types. + * Send in binary if requested and type has suitable send function. */ - if (binary && - OidIsValid(typclass->typsend) && - (att->atttypid < FirstGenbkiObjectId || - (typclass->typtype != TYPTYPE_COMPOSITE && - typclass->typelem == InvalidOid))) + if (binary && OidIsValid(typclass->typsend)) { bytea *outputbytes; int len; diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c index 800107d4e72..392445ea032 100644 --- a/src/backend/utils/adt/arrayfuncs.c +++ b/src/backend/utils/adt/arrayfuncs.c @@ -1308,13 +1308,34 @@ array_recv(PG_FUNCTION_ARGS) (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION), errmsg("invalid array flags"))); + /* Check element type recorded in the data */ element_type = pq_getmsgint(buf, sizeof(Oid)); + + /* + * From a security standpoint, it doesn't matter whether the input's + * element type matches what we expect: the element type's receive + * function has to be robust enough to cope with invalid data. However, + * from a user-friendliness standpoint, it's nicer to complain about type + * mismatches than to throw "improper binary format" errors. But there's + * a problem: only built-in types have OIDs that are stable enough to + * believe that a mismatch is a real issue. So complain only if both OIDs + * are in the built-in range. Otherwise, carry on with the element type + * we "should" be getting. + */ if (element_type != spec_element_type) { - /* XXX Can we allow taking the input element type in any cases? */ - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("wrong element type"))); + if (element_type < FirstGenbkiObjectId && + spec_element_type < FirstGenbkiObjectId) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("binary data has array element type %u (%s) instead of expected %u (%s)", + element_type, + format_type_extended(element_type, -1, + FORMAT_TYPE_ALLOW_INVALID), + spec_element_type, + format_type_extended(spec_element_type, -1, + FORMAT_TYPE_ALLOW_INVALID)))); + element_type = spec_element_type; } for (i = 0; i < ndim; i++) diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c index 80cba2f4c26..674cf0a55d8 100644 --- a/src/backend/utils/adt/rowtypes.c +++ b/src/backend/utils/adt/rowtypes.c @@ -551,13 +551,33 @@ record_recv(PG_FUNCTION_ARGS) continue; } - /* Verify column datatype */ + /* Check column type recorded in the data */ coltypoid = pq_getmsgint(buf, sizeof(Oid)); - if (coltypoid != column_type) + + /* + * From a security standpoint, it doesn't matter whether the input's + * column type matches what we expect: the column type's receive + * function has to be robust enough to cope with invalid data. + * However, from a user-friendliness standpoint, it's nicer to + * complain about type mismatches than to throw "improper binary + * format" errors. But there's a problem: only built-in types have + * OIDs that are stable enough to believe that a mismatch is a real + * issue. So complain only if both OIDs are in the built-in range. + * Otherwise, carry on with the column type we "should" be getting. + */ + if (coltypoid != column_type && + coltypoid < FirstGenbkiObjectId && + column_type < FirstGenbkiObjectId) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("wrong data type: %u, expected %u", - coltypoid, column_type))); + errmsg("binary data has type %u (%s) instead of expected %u (%s) in record column %d", + coltypoid, + format_type_extended(coltypoid, -1, + FORMAT_TYPE_ALLOW_INVALID), + column_type, + format_type_extended(column_type, -1, + FORMAT_TYPE_ALLOW_INVALID), + i + 1))); /* Get and check the item length */ itemlen = pq_getmsgint(buf, 4);