From ec6a8e551fe76d53cff55693952063a813ba0723 Mon Sep 17 00:00:00 2001 From: Kristian Lejao <1741885+lejaokri@users.noreply.github.com> Date: Wed, 18 Oct 2023 08:51:43 -0700 Subject: [PATCH] Another batch of memory management improvement (#1925) Some codepaths access arrays out of bounds or reference uninitialize variables. do_compile() prepare_format_string() pre_transform_target_entry() bbf_ProcessUtility() babelfish_add_domain_mapping_entry_internal() object_name() TdsSendTypeBinary() modify_insert_stmt() Tasks: BABEL-4446, BABEL-4457, BABEL-4456, BABEL-4471, BABEL-4472, BABEL-4483 Signed-off-by: Kristian Lejao --- .../src/backend/tds/tdstypeio.c | 38 +++++++++++++------ contrib/babelfishpg_tsql/runtime/functions.c | 32 ++++++++-------- contrib/babelfishpg_tsql/src/hooks.c | 21 ++++++---- contrib/babelfishpg_tsql/src/pl_comp.c | 11 +++++- contrib/babelfishpg_tsql/src/pl_handler.c | 2 +- contrib/babelfishpg_tsql/src/string.c | 3 +- 6 files changed, 66 insertions(+), 41 deletions(-) diff --git a/contrib/babelfishpg_tds/src/backend/tds/tdstypeio.c b/contrib/babelfishpg_tds/src/backend/tds/tdstypeio.c index ab8f04e68e..daf1ac51de 100644 --- a/contrib/babelfishpg_tds/src/backend/tds/tdstypeio.c +++ b/contrib/babelfishpg_tds/src/backend/tds/tdstypeio.c @@ -2605,10 +2605,13 @@ TdsSendTypeBinary(FmgrInfo *finfo, Datum value, void *vMetaData) maxLen = 0; bytea *vlena = DatumGetByteaPCopy(value); bytea *buf; + int copyLen = 0; TdsColumnMetaData *col = (TdsColumnMetaData *) vMetaData; maxLen = col->metaEntry.type7.maxSize; - buf = (bytea *) palloc0(sizeof(bytea) * maxLen); + copyLen = Max((sizeof(bytea) * maxLen), VARSIZE_ANY_EXHDR(vlena)); + + buf = (bytea *) palloc0(copyLen); memcpy(buf, VARDATA_ANY(vlena), VARSIZE_ANY_EXHDR(vlena)); if ((rc = TdsPutUInt16LE(maxLen)) == 0) @@ -3044,6 +3047,7 @@ TdsSendTypeNumeric(FmgrInfo *finfo, Datum value, void *vMetaData) TdsColumnMetaData *col = (TdsColumnMetaData *) vMetaData; uint8_t max_scale = col->metaEntry.type5.scale; uint8_t max_precision = col->metaEntry.type5.precision; + int target_precision = 0; out = OutputFunctionCall(finfo, value); if (out[0] == '-') @@ -3080,24 +3084,34 @@ TdsSendTypeNumeric(FmgrInfo *finfo, Datum value, void *vMetaData) if (scale == -1) scale = 0; + /* Perform the overflow check before scribbling on to decString. */ + target_precision = precision + (max_scale - scale); + if (target_precision > TDS_MAX_NUM_PRECISION || + target_precision > max_precision) + ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("Arithmetic overflow error for data type numeric."))); + /* - * Fill in the remaining 0's if the processed scale from out is less than - * max_scale This is needed because the output generated by engine may not - * always produce the same precision/scale as calculated by - * resolve_numeric_typmod_from_exp, which is the precision/scale we have - * sent to the client with column metadata. - */ + * Fill in the remaining 0's if the processed scale from out is less than + * max_scale This is needed because the output generated by engine may not + * always produce the same precision/scale as calculated by + * resolve_numeric_typmod_from_exp, which is the precision/scale we have + * sent to the client with column metadata. + */ while (scale++ < max_scale) { decString[precision++] = '0'; } decString[precision] = '\0'; - Assert(precision <= outlen); + Assert(precision == target_precision); - if (precision > TDS_MAX_NUM_PRECISION || - precision > max_precision) - ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), - errmsg("Arithmetic overflow error for data type numeric."))); + /* + * Verify that we did not go beyond the memory allocated. + * We allow precision < outlen. Consider the case when + * out="123.456", max_scale=8. Then by the end, precision=11 + * but outlen=15. + */ + Assert(precision <= outlen); if (precision >= 1 && precision < 10) length = 4; diff --git a/contrib/babelfishpg_tsql/runtime/functions.c b/contrib/babelfishpg_tsql/runtime/functions.c index 35c47498b4..439c68aa97 100644 --- a/contrib/babelfishpg_tsql/runtime/functions.c +++ b/contrib/babelfishpg_tsql/runtime/functions.c @@ -1344,7 +1344,7 @@ object_name(PG_FUNCTION_ARGS) SysScanDesc tgscan; EphemeralNamedRelation enr; bool found = false; - char *result = NULL; + text *result_text = NULL; if (input1 < 0) PG_RETURN_NULL(); @@ -1376,9 +1376,7 @@ object_name(PG_FUNCTION_ARGS) enr = get_ENR_withoid(currentQueryEnv, object_id, ENR_TSQL_TEMP); if (enr != NULL && enr->md.enrtype == ENR_TSQL_TEMP) { - result = enr->md.name; - - PG_RETURN_VARCHAR_P((VarChar *) cstring_to_text(result)); + PG_RETURN_VARCHAR_P((VarChar *) cstring_to_text(enr->md.name)); } /* search in pg_class by object_id */ @@ -1389,8 +1387,7 @@ object_name(PG_FUNCTION_ARGS) if (pg_class_aclcheck(object_id, user_id, ACL_SELECT) == ACLCHECK_OK) { Form_pg_class pg_class = (Form_pg_class) GETSTRUCT(tuple); - result = NameStr(pg_class->relname); - + result_text = cstring_to_text(NameStr(pg_class->relname)); // make a copy before releasing syscache schema_id = pg_class->relnamespace; } ReleaseSysCache(tuple); @@ -1407,8 +1404,7 @@ object_name(PG_FUNCTION_ARGS) if (pg_proc_aclcheck(object_id, user_id, ACL_EXECUTE) == ACLCHECK_OK) { Form_pg_proc procform = (Form_pg_proc) GETSTRUCT(tuple); - result = NameStr(procform->proname); - + result_text = cstring_to_text(NameStr(procform->proname)); schema_id = procform->pronamespace; } ReleaseSysCache(tuple); @@ -1426,7 +1422,7 @@ object_name(PG_FUNCTION_ARGS) if (pg_type_aclcheck(object_id, user_id, ACL_USAGE) == ACLCHECK_OK) { Form_pg_type pg_type = (Form_pg_type) GETSTRUCT(tuple); - result = NameStr(pg_type->typname); + result_text = cstring_to_text(NameStr(pg_type->typname)); } ReleaseSysCache(tuple); found = true; @@ -1454,8 +1450,7 @@ object_name(PG_FUNCTION_ARGS) if (OidIsValid(pg_trigger->tgrelid) && pg_class_aclcheck(pg_trigger->tgrelid, user_id, ACL_SELECT) == ACLCHECK_OK) { - result = NameStr(pg_trigger->tgname); - + result_text = cstring_to_text(NameStr(pg_trigger->tgname)); schema_id = get_rel_namespace(pg_trigger->tgrelid); } found = true; @@ -1475,8 +1470,7 @@ object_name(PG_FUNCTION_ARGS) /* check if user have right permission on object */ if (OidIsValid(con->conrelid) && (pg_class_aclcheck(con->conrelid, user_id, ACL_SELECT) == ACLCHECK_OK)) { - result = NameStr(con->conname); - + result_text = cstring_to_text(NameStr(con->conname)); schema_id = con->connamespace; } ReleaseSysCache(tuple); @@ -1484,7 +1478,7 @@ object_name(PG_FUNCTION_ARGS) } } - if (result) + if (result_text) { /* * Check if schema corresponding to found object belongs to specified @@ -1492,9 +1486,13 @@ object_name(PG_FUNCTION_ARGS) * "information_schema_tsql". In case of pg_type schema_id will be * invalid. */ - if (!OidIsValid(schema_id) || is_schema_from_db(schema_id, database_id) - || (schema_id == get_namespace_oid("sys", true)) || (schema_id == get_namespace_oid("information_schema_tsql", true))) - PG_RETURN_VARCHAR_P((VarChar *) cstring_to_text(result)); + if (!OidIsValid(schema_id) || + is_schema_from_db(schema_id, database_id) || + (schema_id == get_namespace_oid("sys", true)) || + (schema_id == get_namespace_oid("information_schema_tsql", true))) + { + PG_RETURN_VARCHAR_P((VarChar *) result_text); + } } PG_RETURN_NULL(); } diff --git a/contrib/babelfishpg_tsql/src/hooks.c b/contrib/babelfishpg_tsql/src/hooks.c index 47d25d830a..0ce6171cb6 100644 --- a/contrib/babelfishpg_tsql/src/hooks.c +++ b/contrib/babelfishpg_tsql/src/hooks.c @@ -1398,15 +1398,16 @@ pre_transform_target_entry(ResTarget *res, ParseState *pstate, if(actual_alias_len > alias_len) { /* First 32 characters of original_name are assigned to alias. */ - memcpy(alias, original_name, (alias_len - 32) ); + memcpy(alias, original_name, (alias_len - 32)); + /* Last 32 characters of identifier_name are assigned to alias, as actual alias is truncated. */ - memcpy(alias + (alias_len) - 32, - identifier_name + (alias_len) - 32, - 32); + memcpy(alias + (alias_len - 32), + identifier_name + (alias_len - 32), + 32); + alias[alias_len] = '\0'; } - /* Identifier is not truncated. */ - else + else /* Identifier is not truncated. */ { memcpy(alias, original_name, actual_alias_len); } @@ -2274,7 +2275,13 @@ modify_insert_stmt(InsertStmt *stmt, Oid relid) if (att->attnum > 0) { - col->name = NameStr(att->attname); + /* + * Do a deep copy of attname because tuple is a pointer + * to a shared_buffer page which is released when scan + * is ended. + */ + col->name = pstrdup(NameStr(att->attname)); + col->indirection = NIL; col->val = NULL; col->location = 1; diff --git a/contrib/babelfishpg_tsql/src/pl_comp.c b/contrib/babelfishpg_tsql/src/pl_comp.c index 78fd303354..a4d4150056 100644 --- a/contrib/babelfishpg_tsql/src/pl_comp.c +++ b/contrib/babelfishpg_tsql/src/pl_comp.c @@ -465,6 +465,7 @@ do_compile(FunctionCallInfo fcinfo, PLtsql_type *argdtype; PLtsql_variable *argvariable; PLtsql_nsitem_type argitemtype; + int typmod = 0; /* Create $n name for variable */ snprintf(buf, sizeof(buf), "$%d", i + 1); @@ -496,9 +497,15 @@ do_compile(FunctionCallInfo fcinfo, function->is_mstvf = true; } - /* Create datatype info */ + /* + * Create datatype info. + * typmods array has length procStruct->pronargs and numargs >= procStruct->pronargs, + * (See Assert in get_func_arg_info()) + * Pass in typmods[i] while we are not out of bounds, otherwise pass in -1. + */ + typmod = (typmods && i < procStruct->pronargs) ? typmods[i] : -1; argdtype = pltsql_build_datatype(argtypeid, - (typmods ? typmods[i] : -1), + typmod, function->fn_input_collation, NULL); diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 84746b5a35..43a92d3c69 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3172,7 +3172,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, Constraint *con; con = get_rowversion_default_constraint(makeTypeNameFromOid(attr->atttypid, attr->atttypmod)); - rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault)); + rawEnt = (RawColumnDefault *) palloc0(sizeof(RawColumnDefault)); rawEnt->attnum = attr_num + 1; rawEnt->raw_default = (Node *) con->raw_expr; AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, diff --git a/contrib/babelfishpg_tsql/src/string.c b/contrib/babelfishpg_tsql/src/string.c index 9c2b678231..ade657ab9a 100644 --- a/contrib/babelfishpg_tsql/src/string.c +++ b/contrib/babelfishpg_tsql/src/string.c @@ -387,8 +387,6 @@ prepare_format_string(StringInfo buf, char *msg_string, int nargs, memset(fmt_seg, 0, prev_fmt_seg_sz); strncpy(fmt_seg, seg_start, seg_len); prev_fmt_seg_sz = seg_len; - - arg = args[i]; if (i >= nargs || argisnull[i]) { @@ -397,6 +395,7 @@ prepare_format_string(StringInfo buf, char *msg_string, int nargs, continue; } + arg = args[i]; typid = argtypes[i]; type = TypeCategory(typid);