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
Attachments:
trunk.difftext/x-patch; charset=US-ASCII; name=trunk.diffDownload
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** DropErrorMsgWrongType(const char *relnam
*** 599,609 ****
/* wrongkind could be something we don't have in our table... */
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(rentry->nota_msg, relname),
! (wentry->kind != '\0') ? errhint(wentry->drophint_msg) : 0));
}
/*
* RemoveRelations
* Implements DROP TABLE, DROP INDEX, DROP SEQUENCE, DROP VIEW
--- 599,609 ----
/* wrongkind could be something we don't have in our table... */
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(rentry->nota_msg, relname),
! (wentry->kind != '\0') ? errhint("%s", wentry->drophint_msg) : 0));
}
/*
* RemoveRelations
* Implements DROP TABLE, DROP INDEX, DROP SEQUENCE, DROP VIEW
8_3.difftext/x-patch; charset=US-ASCII; name=8_3.diffDownload
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** ConvertTriggerToFK(CreateTrigStmt *stmt,
*** 598,608 ****
MemoryContext oldContext;
ereport(NOTICE,
(errmsg("ignoring incomplete trigger group for constraint \"%s\" %s",
constr_name, buf.data),
! errdetail(funcdescr[funcnum])));
oldContext = MemoryContextSwitchTo(TopMemoryContext);
info = (OldTriggerInfo *) palloc0(sizeof(OldTriggerInfo));
info->args = copyObject(stmt->args);
info->funcoids[funcnum] = funcoid;
info_list = lappend(info_list, info);
--- 598,608 ----
MemoryContext oldContext;
ereport(NOTICE,
(errmsg("ignoring incomplete trigger group for constraint \"%s\" %s",
constr_name, buf.data),
! errdetail("%s", funcdescr[funcnum])));
oldContext = MemoryContextSwitchTo(TopMemoryContext);
info = (OldTriggerInfo *) palloc0(sizeof(OldTriggerInfo));
info->args = copyObject(stmt->args);
info->funcoids[funcnum] = funcoid;
info_list = lappend(info_list, info);
*************** ConvertTriggerToFK(CreateTrigStmt *stmt,
*** 614,624 ****
{
/* Second trigger of set */
ereport(NOTICE,
(errmsg("ignoring incomplete trigger group for constraint \"%s\" %s",
constr_name, buf.data),
! errdetail(funcdescr[funcnum])));
}
else
{
/* OK, we have a set, so make the FK constraint ALTER TABLE cmd */
AlterTableStmt *atstmt = makeNode(AlterTableStmt);
--- 614,624 ----
{
/* Second trigger of set */
ereport(NOTICE,
(errmsg("ignoring incomplete trigger group for constraint \"%s\" %s",
constr_name, buf.data),
! errdetail("%s", funcdescr[funcnum])));
}
else
{
/* OK, we have a set, so make the FK constraint ALTER TABLE cmd */
AlterTableStmt *atstmt = makeNode(AlterTableStmt);
*************** ConvertTriggerToFK(CreateTrigStmt *stmt,
*** 626,636 ****
FkConstraint *fkcon = makeNode(FkConstraint);
ereport(NOTICE,
(errmsg("converting trigger group into constraint \"%s\" %s",
constr_name, buf.data),
! errdetail(funcdescr[funcnum])));
if (funcnum == 2)
{
/* This trigger is on the FK table */
atstmt->relation = stmt->relation;
if (stmt->constrrel)
--- 626,636 ----
FkConstraint *fkcon = makeNode(FkConstraint);
ereport(NOTICE,
(errmsg("converting trigger group into constraint \"%s\" %s",
constr_name, buf.data),
! errdetail("%s", funcdescr[funcnum])));
if (funcnum == 2)
{
/* This trigger is on the FK table */
atstmt->relation = stmt->relation;
if (stmt->constrrel)
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** DropErrorMsgWrongType(char *relname, cha
*** 127,137 ****
/* wrongkind could be something we don't have in our table... */
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(rentry->nota_msg, relname),
! (wentry->kind != '\0') ? errhint(wentry->drophint_msg) : 0));
}
/*
* Emit the right error message for a "DROP" command issued on a
* non-existent relation
--- 127,137 ----
/* wrongkind could be something we don't have in our table... */
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(rentry->nota_msg, relname),
! (wentry->kind != '\0') ? errhint("%s", wentry->drophint_msg) : 0));
}
/*
* Emit the right error message for a "DROP" command issued on a
* non-existent relation
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** set_config_option(const char *name, cons
*** 4423,4433 ****
{
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
! hintmsg ? errhint(hintmsg) : 0));
return false;
}
if (newval < conf->min || newval > conf->max)
{
ereport(elevel,
--- 4423,4433 ----
{
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
! hintmsg ? errhint("%s", hintmsg) : 0));
return false;
}
if (newval < conf->min || newval > conf->max)
{
ereport(elevel,
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*************** describeOneTableDetails(const char *sche
*** 1289,1299 ****
if (add_tablespace_footer('i',
atooid(PQgetvalue(result1, i, 6)),
footers, &count_footers, tmpbuf, false))
{
appendPQExpBuffer(&buf, ", ");
! appendPQExpBuffer(&buf, tmpbuf.data);
count_footers -= 2;
}
else
count_footers -= 1;
--- 1289,1299 ----
if (add_tablespace_footer('i',
atooid(PQgetvalue(result1, i, 6)),
footers, &count_footers, tmpbuf, false))
{
appendPQExpBuffer(&buf, ", ");
! appendPQExpBuffer(&buf, "%s", tmpbuf.data);
count_footers -= 2;
}
else
count_footers -= 1;
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
argumentsREL8_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 Jan 17, 2009 3:34pm, Peter Eisentraut <peter_e@gmx.net> wrote:
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.
Yeah, you're right. I'm using gcc 4.3.2 on Ubuntu 8.10, which uses
-Wformat-security by default.
Alan
Import Notes
Resolved by subject fallback
One thing to watch out for is that the intention may have been to allow
the strings to be translated.
regards, tom lane
I'm not sure if that's the case. How does one find out?
Alan
Import Notes
Resolved by subject fallback
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
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.481Will 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.481Will 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.481Will 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