Re: printf format selection vs. reality

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: printf format selection vs. reality
Date: 2018-05-23 20:28:55
Message-ID: 8383.1527107335@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> On 2018-May-23, Tom Lane wrote:
>> The practical alternatives seem to be to avoid %z in frontend code,
>> or to invent a macro SIZE_T_MODIFIER and use it like INT64_MODIFIER.
>> Either one will be extremely error-prone, I'm afraid, unless we can
>> find a way to get compiler warnings for violations.

> Usage of %z outside safe-known seems really limited. It would be sad to
> force SIZE_T_MODIFIER for elog calls (where it is prevalent) just for
> the benefit of usage outside of elog on fringe platforms -- you're right
> that we do have a few cases of %z under fprintf() already. The good
> news is that AFAICS those strings are not translatable today, so
> changing only those to SIZE_T_MODIFIER (and leaving alone those using
> elog) is maybe not such a big deal. I think those are dshash.c, dsa.c,
> slab.c and aset.c only.

Yeah, I just went through things myself, and concluded that right now
the only hazards are in debug code such as dsa_dump(). So I think that
(a) we don't have a problem we have to fix right now, and (b) going
over to SIZE_T_MODIFIER seems like more trouble than it'd be worth.
Still, this seems like something that will certainly bite us eventually
if we don't install some kind of check.

> (I assume without checking that with the stringinfo API it's OK to use %z).

It is, that goes to snprintf.

> Can't we raise warnings on such usages with an archetype change? (Hm,
> is it possible to change archetype for fprintf?)

The problem is to get a compiler that thinks that %z is a violation
of *any* archetype. gaur's compiler does think that, but it has no
archetype that does accept %z, so that's little help (I've had it
building with -Wno-format since we added %z).

It might be possible for me to install a fractionally-newer compiler
on gaur's host and get usable warnings that way. I think however
that a more practical approach is likely to be to depend on the
Windows/gcc buildfarm members, where (if gcc is correctly installed
and doing what it's supposed to) we should find that %z is accepted
by the gnu_printf archetype but not the plain printf one. So I wish
somebody would try out the patch in <2975(dot)1526862605(at)sss(dot)pgh(dot)pa(dot)us>
on MinGW. It would also be good to find out whether MSVC can be
persuaded to check printf strings.

regards, tom lane

PS: per the above, the patch in <2975(dot)1526862605(at)sss(dot)pgh(dot)pa(dot)us>
would need to be adjusted to use gnu_printf on the stringinfo
functions, if we don't want complaints about %z. This opens
the question of whether we want to allow %m there ...

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christoph Berg 2018-05-23 20:45:26 Re: PG11 jit failing on ppc64el
Previous Message Alvaro Herrera 2018-05-23 20:10:46 Re: printf format selection vs. reality