Re: [PATCHES] appendStringInfoString() micro-opt
Neil Conway <[EMAIL PROTECTED]> writes: > It occurred to me that there is a potential security problem with code > like: > char *my_str; > my_str = read_from_an_untrusted_source(); > appendStringInfo(buf, my_str); > If my_str contains any formatting characters, this crashes the > backend. I'm not sure if there are any actual exploitable instances of > this in the backend, but the above unsafe coding practise is fairly > common. It is? I thought I'd gone around and checked for that. If you see any remaining cases then I'd say they are must-fix items. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] appendStringInfoString() micro-opt
Neil Conway <[EMAIL PROTECTED]> writes: > I'll put this on the back-burner for now, and repost a complete > patch later if I get around to it. I've applied the following patch (since I'd already gone ahead and done the work) that replaces appendStringInfo(buf, "%s", str) with appendStringInfoString(buf, str) It occurred to me that there is a potential security problem with code like: char *my_str; my_str = read_from_an_untrusted_source(); appendStringInfo(buf, my_str); If my_str contains any formatting characters, this crashes the backend. I'm not sure if there are any actual exploitable instances of this in the backend, but the above unsafe coding practise is fairly common. -Neil Index: src/backend/commands/explain.c === RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/explain.c,v retrieving revision 1.118 diff -c -r1.118 explain.c *** src/backend/commands/explain.c 29 Nov 2003 19:51:47 - 1.118 --- src/backend/commands/explain.c 31 Jan 2004 04:49:13 - *** *** 1002,1008 /* And add to str */ if (keyno > 0) appendStringInfo(str, ", "); ! appendStringInfo(str, "%s", exprstr); } appendStringInfo(str, "\n"); --- 1002,1008 /* And add to str */ if (keyno > 0) appendStringInfo(str, ", "); ! appendStringInfoString(str, exprstr); } appendStringInfo(str, "\n"); Index: src/backend/nodes/outfuncs.c === RCS file: /var/lib/cvs/pgsql-server/src/backend/nodes/outfuncs.c,v retrieving revision 1.231 diff -c -r1.231 outfuncs.c *** src/backend/nodes/outfuncs.c 22 Jan 2004 00:34:31 - 1.231 --- src/backend/nodes/outfuncs.c 31 Jan 2004 04:49:38 - *** *** 1427,1433 * We assume the value is a valid numeric literal and so does * not need quoting. */ ! appendStringInfo(str, "%s", value->val.str); break; case T_String: appendStringInfoChar(str, '"'); --- 1427,1433 * We assume the value is a valid numeric literal and so does * not need quoting. */ ! appendStringInfoString(str, value->val.str); break; case T_String: appendStringInfoChar(str, '"'); *** *** 1436,1442 break; case T_BitString: /* internal representation already has leading 'b' */ ! appendStringInfo(str, "%s", value->val.str); break; default: elog(ERROR, "unrecognized node type: %d", (int) value->type); --- 1436,1442 break; case T_BitString: /* internal representation already has leading 'b' */ ! appendStringInfoString(str, value->val.str); break; default: elog(ERROR, "unrecognized node type: %d", (int) value->type); Index: src/backend/utils/adt/regproc.c === RCS file: /var/lib/cvs/pgsql-server/src/backend/utils/adt/regproc.c,v retrieving revision 1.85 diff -c -r1.85 regproc.c *** src/backend/utils/adt/regproc.c 29 Nov 2003 19:51:59 - 1.85 --- src/backend/utils/adt/regproc.c 31 Jan 2004 04:49:56 - *** *** 340,346 if (i > 0) appendStringInfoChar(&buf, ','); ! appendStringInfo(&buf, "%s", format_type_be(thisargtype)); } appendStringInfoChar(&buf, ')'); --- 340,346 if (i > 0) appendStringInfoChar(&buf, ','); ! appendStringInfoString(&buf, format_type_be(thisargtype)); } appendStringInfoChar(&buf, ')'); Index: src/backend/utils/adt/ruleutils.c === RCS file: /var/lib/cvs/pgsql-server/src/backend/utils/adt/ruleutils.c,v retrieving revision 1.161 diff -c -r1.161 ruleutils.c *** src/backend/utils/adt/ruleutils.c 28 Dec 2003 21:57:37 - 1.161 --- src/backend/utils/adt/ruleutils.c 31 Jan 2004 04:55:19 - *** *** 752,758 attname = get_relid_attribute_name(indrelid, attnum); if (!colno || colno == keyno + 1) ! appendStringInfo(&buf, "%s", quote_identifier(attname)); keycoltype = get_atttype(indrelid, attnum); } else --- 752,758 attname = get_relid_attribute_name(indrelid, attnum); if (!colno || colno == keyno + 1) ! appendStringInfoString(&buf, quote_identifier(attname)); keycoltype = get_atttype(indrelid, attnum); } else *** *** 772,778 /* Need parens if it's not a bare function call */ if (indexkey && IsA(indexkey, FuncExpr) && ((FuncExpr *) indexkey)->funcformat == COERCE_EXPLICIT_CALL) ! appendStringInfo(&buf, "%s", str); else appendStringInfo(&buf, "(%s)", str); } --- 772,778 /* Need parens if it's not a bare function call */ if (indexkey && IsA(indexkey, FuncExpr) && ((FuncExpr *) indexkey)->funcformat == COERCE_EXPLICIT_CALL) ! appendStringInfoString(&buf, str); else appendStringInfo(&buf
Re: [PATCHES] appendStringInfoString() micro-opt
Bruce Momjian <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> I'm not objecting to your doing it, exactly, just suggesting that there >> are better things to spend your time on. > Of course, if it makes the code clearer, that is a win in itself. Sure, but I can't see that there's any gain in readability here ... regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] appendStringInfoString() micro-opt
Tom Lane wrote: > Neil Conway <[EMAIL PROTECTED]> writes: > > This patch replaces a bunch of call sites of appendStringInfo() with > > appendStringInfoString(). > > I doubt this saves enough cycles to be worth doing, but if it floats > your boat ... > > When I'm tempted to make a dubious micro-optimization, I always ask > myself "is it likely that the sum of all machine time saved by this > change will exceed the amount of person-time I am about to put into > making it?" Given the number of places you're talking about touching, > and the fact that I've never seen appendStringInfo placing high on a > profile, I suspect this doesn't pass that test. > > I'm not objecting to your doing it, exactly, just suggesting that there > are better things to spend your time on. Of course, if it makes the code clearer, that is a win in itself. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] appendStringInfoString() micro-opt
Tom Lane <[EMAIL PROTECTED]> writes: > I'm not objecting to your doing it, exactly, just suggesting that > there are better things to spend your time on. Heh, probably true :-) I'll put this on the back-burner for now, and repost a complete patch later if I get around to it. > This I would object to, since it creates a risk of failure if anyone > is incautious enough to write a non-constant argument to > appendStringInfoString. Agreed. (Semi-OT note: allowing the user to extend the compiler to enforce rules like this is a cool idea: http://metacomp.stanford.edu/) -Neil ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] appendStringInfoString() micro-opt
Neil Conway <[EMAIL PROTECTED]> writes: > This patch replaces a bunch of call sites of appendStringInfo() with > appendStringInfoString(). I doubt this saves enough cycles to be worth doing, but if it floats your boat ... When I'm tempted to make a dubious micro-optimization, I always ask myself "is it likely that the sum of all machine time saved by this change will exceed the amount of person-time I am about to put into making it?" Given the number of places you're talking about touching, and the fact that I've never seen appendStringInfo placing high on a profile, I suspect this doesn't pass that test. I'm not objecting to your doing it, exactly, just suggesting that there are better things to spend your time on. > I was tempted to make appendStringInfoString() a macro, since (a) it's > just one line of code (b) I'd expect plenty of compilers to be smart > enough to optimize-out a strlen() on a string-literal arg. The > downside is that it would require that appendStringInfoString() > evaluate its arguments more than once. Any comments on whether this is > worth doing? This I would object to, since it creates a risk of failure if anyone is incautious enough to write a non-constant argument to appendStringInfoString. As soon as you factor any future debugging into the equation, the probability that you've made a net savings of time drops to nil :-(. You have to have a *very* large payback to justify putting that kind of booby-trap into the code, and the payback from this change is not only not large, there's no evidence that it'd even be measurable. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] appendStringInfoString() micro-opt
This patch replaces a bunch of call sites of appendStringInfo() with
appendStringInfoString(). (For those without the code in front of
you, the difference between these two functions is that the former
takes a sprintf-style format string and a variable list of arguments,
whereas the latter just takes a single NUL-terminated string;
therefore, the latter is faster if you're just appending a single
string to the buffer).
For the sake of minimizing how much of my time I've wasted if someone
objects, this patch only changes a portion of the call sites that
could be changed; I'll do the rest before committing the patch.
I was tempted to make appendStringInfoString() a macro, since (a) it's
just one line of code (b) I'd expect plenty of compilers to be smart
enough to optimize-out a strlen() on a string-literal arg. The
downside is that it would require that appendStringInfoString()
evaluate its arguments more than once. Any comments on whether this is
worth doing?
Unless anyone objects, I intend to apply the full version of this
patch within 48 hours.
-Neil
Index: src/backend/commands/explain.c
===
RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/explain.c,v
retrieving revision 1.118
diff -c -r1.118 explain.c
*** src/backend/commands/explain.c 29 Nov 2003 19:51:47 - 1.118
--- src/backend/commands/explain.c 25 Jan 2004 03:32:01 -
***
*** 589,595
planstate->instrument->nloops);
}
else if (es->printAnalyze)
! appendStringInfo(str, " (never executed)");
}
appendStringInfoChar(str, '\n');
--- 589,595
planstate->instrument->nloops);
}
else if (es->printAnalyze)
! appendStringInfoString(str, " (never executed)");
}
appendStringInfoChar(str, '\n');
***
*** 702,709
List *lst;
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, " InitPlan\n");
foreach(lst, planstate->initPlan)
{
SubPlanState *sps = (SubPlanState *) lfirst(lst);
--- 702,709
List *lst;
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, " InitPlan\n");
foreach(lst, planstate->initPlan)
{
SubPlanState *sps = (SubPlanState *) lfirst(lst);
***
*** 711,718
es->rtable = sp->rtable;
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, "-> ");
explain_outNode(str, sp->plan,
sps->planstate,
NULL,
--- 711,718
es->rtable = sp->rtable;
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, "-> ");
explain_outNode(str, sp->plan,
sps->planstate,
NULL,
***
*** 725,732
if (outerPlan(plan))
{
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, " -> ");
explain_outNode(str, outerPlan(plan),
outerPlanState(planstate),
NULL,
--- 725,732
if (outerPlan(plan))
{
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, " -> ");
explain_outNode(str, outerPlan(plan),
outerPlanState(planstate),
NULL,
***
*** 737,744
if (innerPlan(plan))
{
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, " -> ");
explain_outNode(str, innerPlan(plan),
innerPlanState(planstate),
outerPlan(plan),
--- 737,744
if (innerPlan(plan))
{
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, " -> ");
explain_outNode(str, innerPlan(plan),
innerPlanState(planstate),
outerPlan(plan),
***
*** 758,765
Plan *subnode = (Plan *) lfirst(lst);
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, " -> ");
explain_outNode(str, subnode,
appendstate->appendplans[j],
--- 758,765
Plan *subnode = (Plan *) lfirst(lst);
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, " -> ");
explain_outNode(str, subnode,
appendstate->appendplans[j],
***
*** 782,789
es->rtable = rte->subquery->rtable;
for (i = 0; i < indent; i++)
! appendStringInfo(str, " ");
! appendStringInfo(str, " -> ");
explain_outNode(str, subnode,
subquerystate->subplan,
--- 782,789
es->rtable = rte->subquery->rtable;
for (i = 0; i < indent; i++)
! appendStringInfoString(str, " ");
! appendStringInfoString(str, " -> ");
explain_outNode(str, subnode,
subquerystate->subplan,
***
*** 800,807
List *lst;
for (i = 0; i < indent; i++)
