Re: Fixes for compiler warnings

Attached are patches to fix the following compiler warnings that I see when
using gcc 4.3.2.

MASTER warning:
tablecmds.c: In function 'DropErrorMsgWrongType':
tablecmds.c:601: warning: format not a string literal and no format
arguments

REL8_3_STABLE warnings:
utility.c: In function 'DropErrorMsgWrongType':
utility.c:129: warning: format not a string literal and no format arguments
trigger.c: In function 'ConvertTriggerToFK':
trigger.c:600: warning: format not a string literal and no format arguments
trigger.c:616: warning: format not a string literal and no format arguments
trigger.c:628: warning: format not a string literal and no format arguments
guc.c: In function 'set_config_option':
guc.c:4424: warning: format not a string literal and no format arguments
describe.c: In function 'describeOneTableDetails':
describe.c:1294: warning: format not a string literal and no format
arguments

Alan

On Saturday 17 January 2009 11:44:07 Alan Li wrote:
> Attached are patches to fix the following compiler warnings that I see when
> using gcc 4.3.2.
>
> MASTER warning:
> tablecmds.c: In function 'DropErrorMsgWrongType':
> tablecmds.c:601: warning: format not a string literal and no format
> arguments
>
> REL8_3_STABLE warnings:
> utility.c: In function 'DropErrorMsgWrongType':
> utility.c:129: warning: format not a string literal and no format arguments
> trigger.c: In function 'ConvertTriggerToFK':
> trigger.c:600: warning: format not a string literal and no format arguments
> trigger.c:616: warning: format not a string literal and no format arguments
> trigger.c:628: warning: format not a string literal and no format arguments
> guc.c: In function 'set_config_option':
> guc.c:4424: warning: format not a string literal and no format arguments
> describe.c: In function 'describeOneTableDetails':
> describe.c:1294: warning: format not a string literal and no format
> arguments

You apparently have your compiler configured with -Wformat-security. Our code
doesn't do that. I think the cases the warning complains about are fine and
the way the warning is designed is a bit bogus.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:

> You apparently have your compiler configured with -Wformat-security. Our code
> doesn't do that. I think the cases the warning complains about are fine and
> the way the warning is designed is a bit bogus.

Hm, only a bit. You know, we've had precisely this bug at least once not that
long ago. And the way the warning is designed it won't fire any false
positives except in cases that are easily avoided.

There's an argument to be made that the code is easier to audit if you put the
"%s" format string in explicitly too. Even if the current code is correct you
have to trace the variable back up to its source to be sure. If you add the
escape then you can see that the code is safe just from that line of code
alone.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> There's an argument to be made that the code is easier to audit if you put the
> "%s" format string in explicitly too.

Yeah, the risk this is trying to guard against is variables containing
"%" unexpectedly. Even if that's not possible, it requires some work
to verify and it's a bit fragile. I didn't look at the specific cases
yet but in general I think this is a good policy.

One thing to watch out for is that the intention may have been to allow
the strings to be translated.

regards, tom lane

On Sunday 18 January 2009 08:28:51 Tom Lane wrote:
> Yeah, the risk this is trying to guard against is variables containing
> "%" unexpectedly. Even if that's not possible, it requires some work
> to verify and it's a bit fragile. I didn't look at the specific cases
> yet but in general I think this is a good policy.

-Wformat-security warns about

printf(var);

but not about

printf(var, a);

I don't understand that; the crash or exploit potential is pretty much the
same in both cases.

-Wformat-nonliteral warns about both cases. We have legitimate code that
requires this, however.

What would be helpful is a way to individually override the warning for the
rare code where you know what you are doing.

On 2009-01-18, at 09:56, Peter Eisentraut wrote:

> On Sunday 18 January 2009 08:28:51 Tom Lane wrote:
>> Yeah, the risk this is trying to guard against is variables
>> containing
>> "%" unexpectedly. Even if that's not possible, it requires some work
>> to verify and it's a bit fragile. I didn't look at the specific
>> cases
>> yet but in general I think this is a good policy.
>
> -Wformat-security warns about
>
> printf(var);
>
> but not about
>
> printf(var, a);
>
> I don't understand that; the crash or exploit potential is pretty
> much the
> same in both cases.
not at all. First case allows you to pass in var from outside, with
your, well crafted format strings. Please read more about subject,
before you say something that silly.

Grzegorz Jaskiewicz wrote:
> On 2009-01-18, at 09:56, Peter Eisentraut wrote:
>> -Wformat-security warns about
>>
>> printf(var);
>>
>> but not about
>>
>> printf(var, a);
>>
>> I don't understand that; the crash or exploit potential is pretty much
>> the
>> same in both cases.
> not at all. First case allows you to pass in var from outside, with
> your, well crafted format strings. Please read more about subject,
> before you say something that silly.

The point is that if "var" comes from an untrusted source, both forms
are just as dangerous.

I guess that in practice, the first form is more likely to be an oversight.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Sunday 18 January 2009 12:43:46 Grzegorz Jaskiewicz wrote:
> > -Wformat-security warns about
> >
> > printf(var);
> >
> > but not about
> >
> > printf(var, a);
> >
> > I don't understand that; the crash or exploit potential is pretty
> > much the
> > same in both cases.
>
> not at all. First case allows you to pass in var from outside, with
> your, well crafted format strings. Please read more about subject,
> before you say something that silly.

If your premise is that var is passed in from the outside, then the real issue
is the %n placeholder. And then it doesn't matter how many variadic args you
pass.

Peter Eisentraut wrote:

> -Wformat-security warns about
>
> printf(var);
>
> but not about
>
> printf(var, a);
>
> I don't understand that; the crash or exploit potential is pretty much the
> same in both cases.

Not sure this is the reason, but in the first case any risk is trivially
avoided by using puts() or printf("%s", var) instead. So printf(var) is
almost certainly not what you mean.

I think that's a reasonable warning to have enabled, whereas the other
one is more of a "try it sometime, you might find something" kind of
warning.

Jeroen