Thread: pgsql: Fix misplaced right paren bugs in pgstatfuncs.c.

pgsql: Fix misplaced right paren bugs in pgstatfuncs.c.

From
Kevin Grittner
Date:
Fix misplaced right paren bugs in pgstatfuncs.c.

The bug would only show up if the C sockaddr structure contained
zero in the first byte for a valid address; otherwise it would
fail to fail, which is probably why it went unnoticed for so long.

Patch submitted by Joel Jacobson after seeing an article by Andrey
Karpov in which he reports finding this through static code
analysis using PVS-Studio.  While I was at it I moved a definition
of a local variable referenced in the buggy code to a more local
context.

Backpatch to all supported branches.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/a133bf7031ad5b62281b21dbd6b2097d3d400f0d

Modified Files
--------------
src/backend/utils/adt/pgstatfuncs.c |    9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)


Re: pgsql: Fix misplaced right paren bugs in pgstatfuncs.c.

From
Tom Lane
Date:
Kevin Grittner <kgrittn@postgresql.org> writes:
> Fix misplaced right paren bugs in pgstatfuncs.c.
>
> The bug would only show up if the C sockaddr structure contained
> zero in the first byte for a valid address; otherwise it would
> fail to fail, which is probably why it went unnoticed for so long.

Just for the sake of the archives: I think this analysis is wrong.
What we had was equivalent to

     if (memcmp(x, y, 0))

The POSIX spec doesn't actually say in so many words what memcmp
should do for zero length, but a reasonable expectation is that
it should return zero ("equal").  So I think really we're getting
constant false here, independently of what's in the sockaddr.

So for example in pg_stat_get_activity(), the test to see if the address
was all-zero would always fail to fire, and it would then try to see what
the address family was --- and unless the platform had used zero for
AF_INET, AF_INET6, or AF_UNIX, it would then fall through to the "Unknown
address type" case, which would set the output columns to null anyway!
Similarly, in pg_stat_get_backend_client_addr and
pg_stat_get_backend_client_port, a zero ss_family field would lead to
returning NULL which was the intended behavior anyhow.

So the reason that it'd gone unnoticed is that there is in fact no
observable bug.  It's a good catch anyway.

            regards, tom lane