Horton
Menu

Re: C99 compliance for src/port/snprintf.c

Tom Lane <tgl@sss.pgh.pa.us>

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2018-08-15T15:41:46Z
Lists: pgsql-hackers 
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 14, 2018 at 7:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm almost tempted to think that the reasons above make this a
>> back-patchable bug fix.  Comments?

> No objections to changing the behavior.  Have you checked whether
> there are any noticeable performance consequences?

Hard to believe there could be any performance loss.  The normal (non
buffer overflowing) code path gets two additional instructions, storing
zero into .nchars and then adding it to the result.  There would be more
work added to buffer-overflow cases, but we'd surely more than win that
back by avoiding repeated repalloc-and-try-again cycles.

> Back-patching seems a bit aggressive to me considering that the danger
> is hypothetical.  I'd want to have some tangible evidence that
> back-patching was going help somebody. For all we know somebody's got
> an extension which they only use on Windows that happens to be relying
> on the current behavior, although more likely still (IMHO) is that it
> there is little or no code relying on either behavior.

Yeah, it's theoretically possible that there's somebody out there who's
depending on the pre-C99 behavior and never tested their code anywhere
but Windows.  But come on, how likely is that really, compared to the
much more plausible risks I already cited?  It's not even that easy
to imagine how someone would construct code that had such a dependency
while not being outright buggy in the face of buffer overrun.

BTW, independently of whether to back-patch, it strikes me that what
we ought to do in HEAD (after applying this) is to just assume we have
C99-compliant behavior, and rip out the baroque logic in psnprintf
and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.
I don't expect that'd save any really meaningful number of cycles,
but at least it'd buy back the two added instructions mentioned above.
I suppose we could put in a configure check that verifies whether
the system snprintf returns the right value for overrun cases, though
it's hard to believe there are any platforms that pass the 'z' check
and would fail this one.

			regards, tom lane

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove test for VA_ARGS, implied by C99.

  2. Introduce minimal C99 usage to verify compiler support.

  3. Require C99 (and thus MSCV 2013 upwards).

  4. Require a C99-compliant snprintf(), and remove related workarounds.

  5. Try to enable C99 in configure, but do not rely on it (yet).

  6. Make snprintf.c follow the C99 standard for snprintf's result value.

  7. Clean up assorted misuses of snprintf()'s result value.