In the wake of commits 7d8f59577+80aa9848b, Coverity has begun
bleating like this:
*** CID 1659393: Memory - illegal accesses (USE_AFTER_FREE)
/srv/coverity/git/pgsql-git/postgresql/contrib/postgres_fdw/postgres_fdw.c:
4930 in postgresAnalyzeForeignTable()
4924 res = pgfdw_exec_query(conn, sql.data, NULL);
4925 if (PQresultStatus(res) != PGRES_TUPLES_OK)
4926 pgfdw_report_error(ERROR, res, conn, sql.data);
4927
4928 if (PQntuples(res) != 1 || PQnfields(res) != 1)
4929 elog(ERROR, "unexpected result from
deparseAnalyzeSizeSql query");
>>> CID 1659393: Memory - illegal accesses (USE_AFTER_FREE)
>>> Calling "libpqsrv_PQgetvalue" dereferences freed pointer "res->res".
4930 *totalpages = strtoul(PQgetvalue(res, 0, 0), NULL, 10);
4931 PQclear(res);
4932
4933 ReleaseConnection(conn);
4934
4935 return true;
(and similarly in other places in postgres_fdw). Now, this complaint
is nonsense because we won't get past the PQresultStatus and
pgfdw_report_error bit if we don't have a fully valid PGresult.
Evidently, Coverity is not managing to figure out that
pgfdw_report_error(ERROR, ...) doesn't return. That's a bit
surprising because it normally does fairly deep interprocedural
analysis, but the conclusion is hard to avoid. Certainly, any
C compiler that doesn't do deep LTO is not going to figure it
out either. I think we have previously just dismissed similar
Coverity complaints, but it seems like we ought to try to fix
it properly this time.
I can see two, or two-and-a-half, ways to do that:
1. Wrap pgfdw_report_error in a macro that makes use of pg_unreachable
in a way similar to what we've done for elog/ereport. The argument
for this is that we needn't touch the 30-or-so pgfdw_report_error
call sites; the argument against is that those elog/ereport macros
are messy, compiler-dependent, and keep needing changes.
2a. Split pgfdw_report_error into two functions, say
pgfdw_report_error() that hard-wires elevel as ERROR and is
labeled noreturn, and pgfdw_report_noerror() that has an
elevel argument that it asserts is less than ERROR.
2b. As 2a except the two functions are pgfdw_report_error()
and pgfdw_report_warning(), both with hard-wired elevel values.
This'd be sufficient right now, but it's plausible that this path
would lead to needing pgfdw_report_log() and some other variants
in future.
The argument for 2a/2b is that they're really simple and are
unlikely to break in the future. The argument against is that
the changes would create back-patching hazards --- but maybe
it'd be reasonable to back-patch the changes to forestall that.
I'm kind of leaning to doing 2a and back-patching it, but
that's certainly a judgment call. Anyone want to argue
for a different approach?
regards, tom lane
