Re: [HACKERS] Fixes for compiler warnings
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.480r2=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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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.480r2=1.481 Will it work to stick _(hintmsg) around it there? //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Magnus Hagander escribió: For a change like http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c?r1=1.480r2=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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Alvaro Herrera alvhe...@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.480r2=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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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
Re: [HACKERS] Fixes for compiler warnings
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
On Jan 17, 2009 3:34pm, Peter Eisentraut pete...@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
Re: [HACKERS] Fixes for compiler warnings
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
alan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Tom Lane t...@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! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Tom Lane wrote: alan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Gregory Stark st...@enterprisedb.com writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
*** 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
*** 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,
Re: [HACKERS] Fixes for compiler warnings
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Peter Eisentraut pete...@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! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixes for compiler warnings
Gregory Stark st...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
