Making pgfdw_report_error statically analyzable
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
On Mon, Jul 28, 2025 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
I would be fine with any of these, but my order of preference would
probably be #2b-#1-#2a i.e. I like your most-preferred alternative
least. However, it's a very mild preference so I am more than fine if
you want to just go ahead and do #2a as you proposed.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 28, 2025 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
I would be fine with any of these, but my order of preference would
probably be #2b-#1-#2a i.e. I like your most-preferred alternative
least. However, it's a very mild preference so I am more than fine if
you want to just go ahead and do #2a as you proposed.
The difference between 2a and 2b is just cosmetic really, so
I'm fine to go with 2b if that's the consensus.
regards, tom lane
On 2025-Jul-28, Tom Lane wrote:
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.
Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
but complement it with pgfdw_report() that takes the elevel as argument,
asserting that it's less than ERROR? Then the calls look like
pgfdw_report(WARNING, "doo dee");
which makes sense IMO and we don't have to worry about the future.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La experiencia nos dice que el hombre peló millones de veces las patatas,
pero era forzoso admitir la posibilidad de que en un caso entre millones,
las patatas pelarían al hombre" (Ijon Tichy)
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
but complement it with pgfdw_report() that takes the elevel as argument,
asserting that it's less than ERROR? Then the calls look like
pgfdw_report(WARNING, "doo dee");
which makes sense IMO and we don't have to worry about the future.
This is the same as my 2a except for the choice of function name.
I'd be fine with it, but Robert didn't like 2a.
regards, tom lane
On Mon, Jul 28, 2025 at 5:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
but complement it with pgfdw_report() that takes the elevel as argument,
asserting that it's less than ERROR? Then the calls look like
pgfdw_report(WARNING, "doo dee");which makes sense IMO and we don't have to worry about the future.
This is the same as my 2a except for the choice of function name.
I'd be fine with it, but Robert didn't like 2a.
I think I like this a little better than your 2a. It's not a big deal, anyway.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 28, 2025 at 5:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
but complement it with pgfdw_report() that takes the elevel as argument,
asserting that it's less than ERROR? Then the calls look like
pgfdw_report(WARNING, "doo dee");
which makes sense IMO and we don't have to worry about the future.
This is the same as my 2a except for the choice of function name.
I'd be fine with it, but Robert didn't like 2a.
I think I like this a little better than your 2a. It's not a big deal, anyway.
I'll run with Alvaro's suggestion, then.
regards, tom lane
I wrote:
I'll run with Alvaro's suggestion, then.
And done. In the event I realized that there's no point in
back-patching this, because 80aa9848b already changed all
the call sites of pgfdw_report_error, so we're already going
to have issues back-patching any fixes that touch those.
regards, tom lane