From 5592ebac55460866da867df5c783c34e3c9a7cae Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 3 Mar 2014 03:18:51 -0500 Subject: [PATCH] Another round of Coverity fixes Additional non-security issues/improvements spotted by Coverity. In backend/libpq, no sense trying to protect against port->hba being NULL after we've already dereferenced it in the switch() statement. Prevent against possible overflow due to 32bit arithmitic in basebackup throttling (not yet released, so no security concern). Remove nonsensical check of array pointer against NULL in procarray.c, looks to be a holdover from 9.1 and earlier when there were pointers being used but now it's just an array. Remove pointer check-against-NULL in tsearch/spell.c as we had already dereferenced it above (in the strcmp()). Remove dead code from adt/orderedsetaggs.c, isnull is checked immediately after each tuplesort_getdatum() call and if true we return, so no point checking it again down at the bottom. Remove recently added minor error-condition memory leak in pg_regress. --- src/backend/libpq/auth.c | 18 +++++++----------- src/backend/replication/basebackup.c | 3 ++- src/backend/storage/ipc/procarray.c | 9 +++------ src/backend/tsearch/spell.c | 2 +- src/backend/utils/adt/orderedsetaggs.c | 5 +---- src/test/regress/pg_regress.c | 12 +++++++++--- 6 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 9d0c3893c8..f03aa7edc2 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -214,6 +214,7 @@ static void auth_failed(Port *port, int status, char *logdetail) { const char *errstr; + char *cdetail; int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION; /* @@ -273,17 +274,12 @@ auth_failed(Port *port, int status, char *logdetail) break; } - if (port->hba) - { - char *cdetail; - - cdetail = psprintf(_("Connection matched pg_hba.conf line %d: \"%s\""), - port->hba->linenumber, port->hba->rawline); - if (logdetail) - logdetail = psprintf("%s\n%s", logdetail, cdetail); - else - logdetail = cdetail; - } + cdetail = psprintf(_("Connection matched pg_hba.conf line %d: \"%s\""), + port->hba->linenumber, port->hba->rawline); + if (logdetail) + logdetail = psprintf("%s\n%s", logdetail, cdetail); + else + logdetail = cdetail; ereport(FATAL, (errcode(errcode_return), diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index d68a153360..f611f591b6 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -227,7 +227,8 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir) /* Setup and activate network throttling, if client requested it */ if (opt->maxrate > 0) { - throttling_sample = opt->maxrate * 1024 / THROTTLING_FREQUENCY; + throttling_sample = + (int64) opt->maxrate * (int64) 1024 / THROTTLING_FREQUENCY; /* * The minimum amount of time for throttling_sample diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 082115b4ff..eac418442d 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2302,9 +2302,9 @@ MinimumActiveBackends(int min) volatile PGXACT *pgxact = &allPgXact[pgprocno]; /* - * Since we're not holding a lock, need to check that the pointer is - * valid. Someone holding the lock could have incremented numProcs - * already, but not yet inserted a valid pointer to the array. + * Since we're not holding a lock, need to be prepared to deal with + * garbage, as someone could have incremented numPucs but not yet + * filled the structure. * * If someone just decremented numProcs, 'proc' could also point to a * PGPROC entry that's no longer in the array. It still points to a @@ -2312,9 +2312,6 @@ MinimumActiveBackends(int min) * free list and are recycled. Its contents are nonsense in that case, * but that's acceptable for this function. */ - if (proc == NULL) - continue; - if (proc == MyProc) continue; /* do not count myself */ if (pgxact->xid == InvalidTransactionId) diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c index 1bc226d334..530c6eddb8 100644 --- a/src/backend/tsearch/spell.c +++ b/src/backend/tsearch/spell.c @@ -404,7 +404,7 @@ NIAddAffix(IspellDict *Conf, int flag, char flagflags, const char *mask, const c Affix->issimple = 0; Affix->isregis = 1; RS_compile(&(Affix->reg.regis), (type == FF_SUFFIX) ? true : false, - (mask && *mask) ? mask : VoidString); + *mask ? mask : VoidString); } else { diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index e5324e2869..99577a549e 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -585,10 +585,7 @@ percentile_cont_final_common(FunctionCallInfo fcinfo, * trouble, since the cleanup callback will clear the tuplesort later. */ - if (isnull) - PG_RETURN_NULL(); - else - PG_RETURN_DATUM(val); + PG_RETURN_DATUM(val); } /* diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 541ce8b33d..438b95fe59 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1154,11 +1154,17 @@ get_alternative_expectfile(const char *expectfile, int i) { char *last_dot; int ssize = strlen(expectfile) + 2 + 1; - char *tmp = (char *) malloc(ssize); - char *s = (char *) malloc(ssize); + char *tmp; + char *s; - if (!tmp || !s) + if (!(tmp = (char*) malloc(ssize))) return NULL; + + if (!(s = (char*) malloc(ssize))) + { + free(tmp); + return NULL; + } strcpy(tmp, expectfile); last_dot = strrchr(tmp, '.');