Обсуждение: 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@gmx.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@enterprisedb.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.

> One thing to watch out for is that the intention may have been to allow<br />> <br />> the strings to be
translated.<br/>> <br />> <br />> <br />>                        regards, tom lane<br />> <br /><br
/>I'mnot sure if that's the case. How does one find out?<br /><br />Alan 

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.

On Jan 17, 2009 3:34pm, Peter Eisentraut <peter_e@gmx.net> wrote:<br />> On Saturday 17 January 2009 11:44:07
AlanLi wrote:<br />> <br />> > Attached are patches to fix the following compiler warnings that I see when<br
/>><br />> > using gcc 4.3.2.<br />> <br />> ><br />> <br />> > MASTER warning:<br />>
<br/>> > tablecmds.c: In function 'DropErrorMsgWrongType':<br />> <br />> > tablecmds.c:601: warning:
formatnot a string literal and no format<br />> <br />> > arguments<br />> <br />> ><br />> <br
/>>> REL8_3_STABLE warnings:<br />> <br />> > utility.c: In function 'DropErrorMsgWrongType':<br />>
<br/>> > utility.c:129: warning: format not a string literal and no format arguments<br />> <br />> >
trigger.c:In function 'ConvertTriggerToFK':<br />> <br />> > trigger.c:600: warning: format not a string
literaland no format arguments<br />> <br />> > trigger.c:616: warning: format not a string literal and no
formatarguments<br />> <br />> > trigger.c:628: warning: format not a string literal and no format
arguments<br/>> <br />> > guc.c: In function 'set_config_option':<br />> <br />> > guc.c:4424:
warning:format not a string literal and no format arguments<br />> <br />> > describe.c: In function
'describeOneTableDetails':<br/>> <br />> > describe.c:1294: warning: format not a string literal and no
format<br/>> <br />> > arguments<br />> <br />> <br />> <br />> You apparently have your compiler
configuredwith -Wformat-security.  Our code<br />> <br />> doesn't do that.  I think the cases the warning
complainsabout are fine and<br />> <br />> the way the warning is designed is a bit bogus.<br />> <br /><br
/>Yeah,you're right. I'm using gcc 4.3.2 on Ubuntu 8.10, which uses -Wformat-security by default.<br /><br />Alan 

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

alanwli@gmail.com writes:
>> One thing to watch out for is that the intention may have been to allow
>> the strings to be translated.

> I'm not sure if that's the case. How does one find out?

If the origin of the "variable" format is a constant or set of constants
decorated with gettext_noop(), then this type of edit will have defeated
the intended localization.  In the cases at hand, I believe that all but
one of your proposed patches break the desired behavior.

What's worse, I see that Magnus got there before you, and has broken
localization here and in several other places:
http://archives.postgresql.org/pgsql-committers/2008-11/msg00264.php

Magnus, you wanna clean up the mess?  And what patch does the "few more"
comment refer back to?

A workable solution that both silences the warning and preserves
localizability is to follow a coding pattern like this:
const char *mymsg = gettext_noop("Some text to be localized.");
...
errmsg("%s", _(mymsg))        // not just errmsg(mymsg)

I would recommend that we do this, because otherwise we are certainly
going to have more breakage from well-intentioned patchers, whatever
Peter's opinion of the merits of the compiler warning might be ;-)

The really nasty cases are like this:
const char *myfmt = gettext_noop("Some bleat about object \"%s\".");
...
errmsg(myfmt, objectname)

where there really is no simple way to convince the compiler that you
know what you're doing without breaking functionality.  This is probably
why -Wformat-security doesn't warn about the latter type of usage.  It
does kind of beg the question of why bother with that warning though ...
        regards, tom lane

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

> The really nasty cases are like this:
>
>     const char *myfmt = gettext_noop("Some bleat about object \"%s\".");
>
>     ...
>
>     errmsg(myfmt, objectname)
>
> where there really is no simple way to convince the compiler that you
> know what you're doing without breaking functionality.  This is probably
> why -Wformat-security doesn't warn about the latter type of usage.  It
> does kind of beg the question of why bother with that warning though ...

It makes sense to me: if you have arguments for the format string then
presumably you've at some point had to check that the format string has
escapes for those arguments.

The only danger in the coding style comes from the possibility that there are
escapes you didn't anticipate. It's a lot harder to expect specific non-zero
escapes and find something else than to just not think about it at all and
unknowingly depend on having no escapes.

And it would take willful ignorance to depend on having some specific set of
escapes in an unchecked string provided by an external data source, which is
where the worst danger lies.


--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services!

Tom Lane wrote:
> alanwli@gmail.com writes:
>>> One thing to watch out for is that the intention may have been to allow
>>> the strings to be translated.
> 
>> I'm not sure if that's the case. How does one find out?
> 
> If the origin of the "variable" format is a constant or set of constants
> decorated with gettext_noop(), then this type of edit will have defeated
> the intended localization.  In the cases at hand, I believe that all but
> one of your proposed patches break the desired behavior.
> 
> What's worse, I see that Magnus got there before you, and has broken
> localization here and in several other places:
> http://archives.postgresql.org/pgsql-committers/2008-11/msg00264.php

> Magnus, you wanna clean up the mess?  

Crap. Yeah, I'll try to get around to that soon. No time tonight though.

> And what patch does the "few more"
> comment refer back to?

I think it refers to this:
http://archives.postgresql.org/pgsql-committers/2008-11/msg00249.php

Initially it came out of this thread:
http://archives.postgresql.org/pgsql-hackers/2008-11/msg01348.php

If my memory is correct, there shouldn't be more than those two patches.


> A workable solution that both silences the warning and preserves
> localizability is to follow a coding pattern like this:
> 
>     const char *mymsg = gettext_noop("Some text to be localized.");
> 
>     ...
> 
>     errmsg("%s", _(mymsg))        // not just errmsg(mymsg)
> 
> I would recommend that we do this, because otherwise we are certainly
> going to have more breakage from well-intentioned patchers, whatever
> Peter's opinion of the merits of the compiler warning might be ;-)

Seems reasonable.

//Magnus

Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> The really nasty cases are like this:
>> const char *myfmt = gettext_noop("Some bleat about object \"%s\".");
>> ...
>> errmsg(myfmt, objectname)

> It makes sense to me: if you have arguments for the format string then
> presumably you've at some point had to check that the format string has
> escapes for those arguments.

Actually, there was just an issue in the open patch for column
privileges where Stephen had added a format string that failed to match
the arguments that would be supplied.  What'd be really useful is some
way to tie the constants themselves to the errmsg call for error
checking purposes ... can't see a decent way to do it though.

BTW, does the gettext infrastructure make any checks to ensure that
translators didn't bollix the format codes?  It seems like that should
be doable with just a SMOP, but I don't know if it's in there or not.
        regards, tom lane

On Sunday 18 January 2009 21:15:28 Tom Lane wrote:
> BTW, does the gettext infrastructure make any checks to ensure that
> translators didn't bollix the format codes?  It seems like that should
> be doable with just a SMOP, but I don't know if it's in there or not.

Yes, that is all taken care of.

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.

Tom Lane wrote:
> Magnus, you wanna clean up the mess?  And what patch does the "few more"
> comment refer back to?
> 
> A workable solution that both silences the warning and preserves
> localizability is to follow a coding pattern like this:
> 
>     const char *mymsg = gettext_noop("Some text to be localized.");
> 
>     ...
> 
>     errmsg("%s", _(mymsg))        // not just errmsg(mymsg)

For a change like
http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.480&r2=1.481

Will it work to stick _(hintmsg) around it there?


//Magnus

Magnus Hagander escribió:

> For a change like
> http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.480&r2=1.481
> 
> Will it work to stick _(hintmsg) around it there?

Assuming that there is a gettext_noop() call in the literal that's
assigned to hintmsg, yes, it should work.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Magnus Hagander escribi�:
>> For a change like
>> http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.480&r2=1.481
>> 
>> Will it work to stick _(hintmsg) around it there?

> Assuming that there is a gettext_noop() call in the literal that's
> assigned to hintmsg, yes, it should work.

... and if there isn't, it's not this code's fault ...
        regards, tom lane

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

Alvaro Herrera wrote:
> Magnus Hagander escribió:
> 
>> For a change like
>> http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.480&r2=1.481
>>
>> Will it work to stick _(hintmsg) around it there?
> 
> Assuming that there is a gettext_noop() call in the literal that's
> assigned to hintmsg, yes, it should work.

Ok, I've applied a fix for this. Hope I got it right ;)

//Magnus