On 13/08/2026 20:24, Tom Lane wrote:
We've had multiple security bugs (CVE-2026-14669, CVE-2015-0241) in
formatting.c due to its habit of using output buffers of predetermined
length, which are usually much too big but sometimes not big enough.
And the path of least resistance for those security fixes was to
impose arbitrary limits on substring lengths, which is surely a wart.
I think it's time to stop the bleeding once and for all, by switching
that code over to using StringInfos for its output buffers.
That turns out to be a good deal more painful than one could wish,
because the code is messy and inconsistent about how it uses its
buffers; but detangling that seemed like a good code cleanup exercise
anyway. So attached is a patch series to accomplish this:
0001: Convert DCH_to_char() to use a StringInfo output buffer.
0002: Restructure NUM_processor() to make it clearer which
string is the output buffer.
0003: Convert NUM_processor() to use StringInfo output buffers.
0004: Try to buy back some performance (see below).
This all looks good to me.
The main problem with this proposal is that it makes these functions
a little slower, apparently because calling snprintf() via
AppendStringInfo() is slower than calling it directly. After the
performance hacking in 0004, what I see is that float8_to_char
and numeric_to_number are the same speed or a little faster than
before, but timestamptz_to_char is still around 10% slower in
a tight-loop benchmark. (See drive_formatting.c, attached, for
the benchmark infrastructure.) Maybe that's okay given that the
overall effect on a complete SQL query should be far less, but
I'd still like to squeeze out a bit more speed. I don't see any
additional low-hanging fruit though.
More aggressive inlining seems like the straightforward solution. The
EMIT macros could do more...
You could replace the appendStringInfo() calls with more specialized
functions. Let's take "HH", for example. You could surely have a more
optimized implementation of that than calling appendStringInfo(out,
"%02d", ...).
With some inline functions and macros, you could probably still have it
read DCH_EMITF("%02d", ...) in the source code, but make the DCH_EMITF()
macro check that it's a compile-time constant and route it to a more
efficient function that gets fully inlined at compile time.
Attached is another micro-optimization that makes a surprisingly big
difference on my laptop (10% - 20%). In a nutshell, have a fast-path for
when a constant character in the format string is a one byte character.
I think it's a good bet that most constants in a format string are
characters like spaces, ":" or "-", which are a single ASCII character
in all locales.
I started with a slightly bigger refactoring to replace the
null-terminated FormatNode->character field with a separate length
field. That's also be pretty straightforward, but when I started to test
it, it turns out that you get the same effect from just the attached.
- Heikki
diff --git a/src/backend/utils/adt/formatting.c
b/src/backend/utils/adt/formatting.c
index 4ae25b90980..9c1e67583ab 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -2594,6 +2594,7 @@ DCH_to_char(const FormatNode *node, bool is_interval, Oid
collid,
#define DCH_EMITF(...) appendStringInfo(out, __VA_ARGS__)
#define DCH_EMITS(str) appendStringInfoString(out, str)
+#define DCH_EMITC(chr) appendStringInfoCharMacro(out, chr)
/* cache localized days and months */
cache_locale_time();
@@ -2604,7 +2605,10 @@ DCH_to_char(const FormatNode *node, bool is_interval,
Oid collid,
if (n->type != NODE_TYPE_ACTION)
{
- DCH_EMITS(n->character);
+ if (n->character[1] == '\0')
+ DCH_EMITC(n->character[0]);
+ else
+ DCH_EMITS(n->character);
continue;
}
