From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Joe Conway <mail@joeconway.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: Incautious handling of overlength identifiers
Date: Mon, 26 Dec 2016 12:05:55 -0500
Message-ID: <6481.1482771955@sss.pgh.pa.us> (raw)
In-Reply-To: <CAB7nPqRa4O0a93kaKaropQqYG283OA5BabRoCS=oGQn16xFTEA@mail.gmail.com>
References: <17691.1482523168@sss.pgh.pa.us>
<20641.1482525852@sss.pgh.pa.us>
<ed23895f-c3b5-e42c-7a79-c676378c34da@joeconway.com>
<CAB7nPqRa4O0a93kaKaropQqYG283OA5BabRoCS=oGQn16xFTEA@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Dec 24, 2016 at 7:44 AM, Joe Conway <mail@joeconway.com> wrote:
>> On 12/23/2016 12:44 PM, Tom Lane wrote:
>>> An alternative worth considering, especially for the back branches,
>>> is simply to remove the Assert in hashname(). That would give us
>>> the behavior that non-developers see anyway, which is that these
>>> functions always fail to match overlength names, whether or not
>>> the names would have matched after truncation. Trying to apply
>>> truncation more consistently could be left as an improvement
>>> project for later.
>> That sounds reasonable to me.
> +1 for just removing the assertion on back-branches. On HEAD, it seems
> right to me to keep the assertion. However it is not possible to just
> switch those routines from text to name as a table could be defined
> with its schema name. So at minimum this would require adjusting
> textToQualifiedNameList() & similar routines in charge of putting in
> shape the name lists.
textToQualifiedNameList() already does truncate, and I suspect everyplace
that deals in qualified names does as well. The problem places are those
where someone just took a text parameter, did text_to_cstring on it, and
started treating the result as a Name (e.g. by passing it to a catalog
lookup function). Since we've intentionally allowed C strings to be used
as Names in most places, it's not even immediately obvious that this is
wrong.
I'm currently inclined to think that removing the assertion from hashname()
is right even for HEAD, because it's not very relevant to the operation
of that routine (you'll get back some hash value even for overlength
strings), and because it's a pretty useless way of enforcing truncation.
It basically will only catch you if you try to do a syscache lookup to
resolve a name --- if you do a catalog heap or index scan, those paths
contain no such gotcha. And I'm disinclined to introduce one.
The real problem with trying to enforce this through length assertions
in low-level routines is that they'll only reveal a bug if you actually
happen to test the appropriate calling code path with an overlength name.
We've obviously failed to do that in the past and I have little faith that
we'd do it in the future.
So that's why I was thinking about whether we could do this through some
datatype-based approach, whereby we could hope to catch incorrect coding
reliably through compiler checks. But given our history of allowing C
strings as names, I'm afraid that any such change would be enormously
invasive and not worth the trouble.
Another idea worth considering is to just make the low-level functions
do truncation, ie the fix in hashname would look more like
- Assert(keylen < NAMEDATALEN);
+ if (keylen >= NAMEDATALEN)
+ keylen = pg_mbcliplen(key, keylen, NAMEDATALEN - 1);
and we'd need something similar in the name comparison functions.
But that would be slightly annoying from a performance standpoint.
Not so much the extra pg_mbcliplen calls, because we could assume
those wouldn't happen in any performance-interesting cases; but
there are no strlen calls in the name comparison functions right now,
so we'd have to add some, and those would get hit every time through.
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, michael.paquier@gmail.com, mail@joeconway.com
Subject: Re: Incautious handling of overlength identifiers
In-Reply-To: <6481.1482771955@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox