From 280416b785744f3c0190f060751a76510b1239ce Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 24 Nov 2016 11:54:06 +0100 Subject: [PATCH] Guard against possible NULL pointer dereferencing Improves defensiveness of programming around pointer derefencing to ensure that we don't risk a NULL pointer. Most of these are quite straight-forward, those of note are discussed below. In doDispatchDtxProtocolCommand() we relied on the result data being created in zeroed out memory on CdbDispatchDtxProtocolCommand() which isn't guaranteed for every compiler. Explcitly set numResults to zero and also check the results for NULL. Per multiple reports by Coverity --- src/backend/cdb/cdbpartindex.c | 95 ++++++++++++------------ src/backend/cdb/cdbtm.c | 4 +- src/backend/cdb/dispatcher/cdbdisp_dtx.c | 2 +- src/bin/pg_dump/cdb/cdb_dump.c | 8 +- src/bin/pg_dump/cdb/cdb_dump_agent.c | 2 + 5 files changed, 60 insertions(+), 51 deletions(-) diff --git a/src/backend/cdb/cdbpartindex.c b/src/backend/cdb/cdbpartindex.c index 9e95cf3d9c..a4b91baeb1 100644 --- a/src/backend/cdb/cdbpartindex.c +++ b/src/backend/cdb/cdbpartindex.c @@ -744,42 +744,42 @@ static void indexParts(PartitionIndexNode **np, bool isDefault) PartitionIndexNode *n = *np; bool found; - if (n) + if (!n) + return; + + int x; + x = bms_first_from(n->index, 0); + while (x >= 0) { - int x; - x = bms_first_from(n->index, 0); - while (x >= 0) + entry = (LogicalIndexInfoHashEntry *)hash_search(LogicalIndexInfoHash, + (void *)&x, + HASH_FIND, + &found); + if (!found) { - entry = (LogicalIndexInfoHashEntry *)hash_search(LogicalIndexInfoHash, - (void *)&x, - HASH_FIND, - &found); - if (!found) - { - ereport(ERROR, - (errcode(ERRCODE_INTERNAL_ERROR), - errmsg("error during BuildLogicalIndexInfo. Indexr not found \"%d\" in hash", - x))); - } + ereport(ERROR, + (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("error during BuildLogicalIndexInfo. Indexr not found \"%d\" in hash", + x))); + } - if (n->isDefault || isDefault) - { - /* - * We keep track of the default node in the PartitionIndexNode - * tree, since we need to output the defaultLevels information - * to the caller. - */ - entry->defaultPartList = lappend(entry->defaultPartList, n); - numIndexesOnDefaultParts++; - } - else - /* - * For regular non-default parts we just track the part oid - * which will be used to get the part constraint. - */ - entry->partList = lappend_oid(entry->partList, n->parchildrelid); - x = bms_first_from(n->index, x + 1); + if (n->isDefault || isDefault) + { + /* + * We keep track of the default node in the PartitionIndexNode + * tree, since we need to output the defaultLevels information + * to the caller. + */ + entry->defaultPartList = lappend(entry->defaultPartList, n); + numIndexesOnDefaultParts++; } + else + /* + * For regular non-default parts we just track the part oid + * which will be used to get the part constraint. + */ + entry->partList = lappend_oid(entry->partList, n->parchildrelid); + x = bms_first_from(n->index, x + 1); } if (n->children) @@ -1229,28 +1229,27 @@ dumpPartsIndexInfo(PartitionIndexNode *n, int level) StringInfoData logicalIndexes; initStringInfo(&logicalIndexes); - if (n) - { - for (int i = 0; i <= level; i++) - appendStringInfo(&logicalIndexes, "%s", " "); - - appendStringInfo(&logicalIndexes, "%d ", n->parchildrelid); + if (!n) + return; - int x; - x = bms_first_from(n->index, 0); - appendStringInfo(&logicalIndexes, "%s ", " ("); + for (int i = 0; i <= level; i++) + appendStringInfo(&logicalIndexes, "%s", " "); - while (x >= 0) - { - appendStringInfo(&logicalIndexes, "%d ", x); - x = bms_first_from(n->index, x + 1); - } + appendStringInfo(&logicalIndexes, "%d ", n->parchildrelid); - appendStringInfo(&logicalIndexes, "%s ", " )"); + int x; + x = bms_first_from(n->index, 0); + appendStringInfo(&logicalIndexes, "%s ", " ("); - elog(DEBUG5, "%s", logicalIndexes.data); + while (x >= 0) + { + appendStringInfo(&logicalIndexes, "%d ", x); + x = bms_first_from(n->index, x + 1); } + appendStringInfo(&logicalIndexes, "%s ", " )"); + + elog(DEBUG5, "%s", logicalIndexes.data); if (n->children) { diff --git a/src/backend/cdb/cdbtm.c b/src/backend/cdb/cdbtm.c index b5a7c0e855..6be1dbce2a 100644 --- a/src/backend/cdb/cdbtm.c +++ b/src/backend/cdb/cdbtm.c @@ -2268,7 +2268,9 @@ doDispatchDtxProtocolCommand(DtxProtocolCommand dtxProtocolCommand, int flags, for (i = 0; i < resultCount; i++) PQclear(results[i]); - free(results); + + if (results) + free(results); return (numOfFailed == 0); } diff --git a/src/backend/cdb/dispatcher/cdbdisp_dtx.c b/src/backend/cdb/dispatcher/cdbdisp_dtx.c index 9215834da3..37b1972636 100644 --- a/src/backend/cdb/dispatcher/cdbdisp_dtx.c +++ b/src/backend/cdb/dispatcher/cdbdisp_dtx.c @@ -82,7 +82,7 @@ CdbDispatchDtxProtocolCommand(DtxProtocolCommand dtxProtocolCommand, CdbDispatcherState ds = {NULL, NULL, NULL}; CdbDispatchResults* pr = NULL; - CdbPgResults cdb_pgresults; + CdbPgResults cdb_pgresults = {NULL, 0}; DispatchCommandDtxProtocolParms dtxProtocolParms; Gang *primaryGang; diff --git a/src/bin/pg_dump/cdb/cdb_dump.c b/src/bin/pg_dump/cdb/cdb_dump.c index 4cd096f663..6e61b0b84d 100644 --- a/src/bin/pg_dump/cdb/cdb_dump.c +++ b/src/bin/pg_dump/cdb/cdb_dump.c @@ -1641,7 +1641,13 @@ reportBackupResults(InputOptions inputopts, ThreadParmArray *pParmAr) SegmentDatabase *pSegDB; ThreadParm *p; - if (pParmAr == NULL || pParmAr->count == 0) + if (pParmAr == NULL) + { + mpp_err_msg(logError, progname, "Report data missing\n"); + return 2; + } + + if (pParmAr->count == 0) { pParmAr->count = 1; /* just use master in this case (early error) */ pParmAr->pData = (ThreadParm *) calloc(1, sizeof(ThreadParm)); diff --git a/src/bin/pg_dump/cdb/cdb_dump_agent.c b/src/bin/pg_dump/cdb/cdb_dump_agent.c index eaa8cc1eb4..305bc09daa 100644 --- a/src/bin/pg_dump/cdb/cdb_dump_agent.c +++ b/src/bin/pg_dump/cdb/cdb_dump_agent.c @@ -8232,12 +8232,14 @@ formGenericFilePathName(char *keyword, char *pszBackupDirectory, char *pszBackup { mpp_err_msg(logWarn, progname, "Backup catalog FileName based on path %s and key %s too long", pszBackupDirectory, pszBackupKey); + exit_nicely(); } pszBackupFileName = (char *) malloc(sizeof(char) * (1 + len)); if (pszBackupFileName == NULL) { mpp_err_msg(logError, progname, "out of memory"); + exit_nicely(); } strcpy(pszBackupFileName, pszBackupDirectory); -- GitLab