From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: Robert Haas <robertmhaas@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: C99 compliance for src/port/snprintf.c
In-reply-to: <13169.1534352739@sss.pgh.pa.us>
References: <20180815155643.zx4qpcmpvhemefyg@alvherre.pgsql>
 <13169.1534352739@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 15 Aug 2018 13:05:39 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17489.1534356329.1@sss.pgh.pa.us>
Date: Wed, 15 Aug 2018 14:05:29 -0400
Message-ID: <17490.1534356329@sss.pgh.pa.us>
Precedence: bulk

I wrote:
> Meh --- the hazards of back-patching seem to me to be more hypothetical
> than the benefits.  Still, I seem to be in the minority, so I withdraw
> the proposal to back-patch.

Actually, after digging around a bit, I'm excited about this again.
There are only a couple dozen places in our tree that pay any attention
to the result of (v)snprintf, but with the exception of psnprintf,
appendPQExpBufferVA, and one or two other places, *they're all assuming
C99 semantics*, and will fail to detect buffer overflow with the pre-C99
behavior.

Probably a lot of these are not live bugs because buffer overrun is
not ever going to occur in practice.  But at least pg_upgrade and
pg_regress are constructing command strings including externally
supplied paths, so overrun doesn't seem impossible.  If it happened,
they'd merrily proceed to execute a truncated command.

If we don't backpatch the snprintf change, we're morally obliged to
back-patch some other fix for these places.  At least one of them,
in plperl's pport.h, is not our code and so changing it seems like
a bad idea.

Still want to argue for no backpatch?

			regards, tom lane

PS: I also found a couple of places that are just wrong regardless
of semantics: they're checking overflow by "result > bufsize", not
"result >= bufsize".  Will fix those in any case.