Horton
Menu

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

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2018-08-15T21:52:17Z
Lists: pgsql-hackers 
On 2018-08-15 14:05:29 -0400, Tom Lane wrote:
> 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.

I'm a bit confused. Why did you just backpatch this ~two hours after
people objected to the idea?  Even if it were during my current work
hours, I don't even read mail that often if I'm hacking on something
complicated.

Greetings,

Andres Freund

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.