Tightening up allowed custom GUC names
Over in [1]/messages/by-id/20210209144059.GA21360@depesz.com it was noted that the system behaves rather oddly if
you try to do ALTER USER/DATABASE SET with a custom GUC name
containing "=" or "-". I think we should just disallow such cases.
Relaxing the restriction is harder than it might seem:
* The convention for entries in pg_db_role_setting is just
"name=value" with no quoting rule, so GUC names containing "="
can't work. We could imagine installing some kind of quoting rule,
but that would break client-side code that looks at this catalog;
pg_dump, for one, does so. On balance it seems clearly not worth
changing that.
* The problem with using "-" is that we parse pg_db_role_setting
entries with ParseLongOption(), which converts "-" to "_" because
that's what makes sense to do in the context of command-line switches
such as "-c work-mem=42MB". We could imagine adjusting the code to
not do that in the pg_db_role_setting case, but you'd still be left
with a GUC that cannot be set via PGOPTIONS="-c custom.my-guc=42".
To avoid that potential confusion, it seems best to ban "-" as well
as "=".
Now granting that the best answer is just to forbid these cases,
there are still a couple of decisions about how extensive the
prohibition ought to be:
* We could forbid these characters only when you try to actually
put such a GUC into pg_db_role_setting, and otherwise allow them.
That seems like a weird nonorthogonal choice though, so I'd
rather just forbid them period.
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.
Anyway, attached is a proposed patch that implements the restriction
as stated. I'm inclined to propose this for HEAD only and not
worry about the issue in the back branches.
Thoughts?
regards, tom lane
Attachments:
restrict-custom-guc-names.patchtext/x-diff; charset=us-ascii; name=restrict-custom-guc-names.patchDownload+104-70
On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
Now granting that the best answer is just to forbid these cases,
there are still a couple of decisions about how extensive the
prohibition ought to be:* We could forbid these characters only when you try to actually
put such a GUC into pg_db_role_setting, and otherwise allow them.
That seems like a weird nonorthogonal choice though, so I'd
rather just forbid them period.
Agreed.
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.
I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.
Noah Misch <noah@leadboat.com> writes:
On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.
I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.
Any other opinions here? I'm hesitant to make such a change on the
basis of just one vote.
regards, tom lane
чц, 11 лют 2021, 21:33 карыстальнік Tom Lane <tgl@sss.pgh.pa.us> напісаў:
Noah Misch <noah@leadboat.com> writes:
On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.Any other opinions here? I'm hesitant to make such a change on the
basis of just one vote.
+1 for the change. I have not seen usage of = and - in the wild in GUC
names but can see a harm of mis-interpretation of these.
Show quoted text
regards, tom lane
On Tue, Feb 9, 2021 at 6:02 PM Noah Misch <noah@leadboat.com> wrote:
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.
I'm not sure exactly what the rule should be here, but in general I
agree that a broader prohibition might be better. It's hard to
understand the rationale behind a system that doesn't allow
robert.max-workers as a GUC name, but does permit ro
b"ert.max^Hworkers.
+1 for not back-patching whatever we do here.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2/11/21 1:32 PM, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.Any other opinions here? I'm hesitant to make such a change on the
basis of just one vote.
That might be a bit restrictive. I could at least see allowing '-' as
reasonable, and maybe ':'. Not sure about other punctuation characters.
OTOH I'd be surprised if the identifier restriction would burden a large
number of people.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Feb 11, 2021 at 02:50:13PM -0500, Robert Haas wrote:
+1 for not back-patching whatever we do here.
+1.
--
Michael[ getting back to this, after a bit of procrastination ]
Andrew Dunstan <andrew@dunslane.net> writes:
On 2/11/21 1:32 PM, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier. I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here. But I could
live with it if people prefer that way.
I'd prefer that. Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.
That might be a bit restrictive. I could at least see allowing '-' as
reasonable, and maybe ':'. Not sure about other punctuation characters.
OTOH I'd be surprised if the identifier restriction would burden a large
number of people.
We can't allow '-', for the specific reason that it won't work as a -c
argument (thanks to -c's translation of '-' to '_'). The whole point here
is to prevent corner cases like that. ':' would be all right, but I think
it's a lot simpler to explain and a lot harder to break in future if we
just say that the names have to be valid identifiers.
Patch that does it like that attached.
(I concur with the downthread opinions that we shouldn't back-patch this.)
regards, tom lane
Attachments:
restrict-custom-guc-names-2.patchtext/x-diff; charset=us-ascii; name=restrict-custom-guc-names-2.patchDownload+132-70
I wrote:
We can't allow '-', for the specific reason that it won't work as a -c
argument (thanks to -c's translation of '-' to '_'). The whole point here
is to prevent corner cases like that. ':' would be all right, but I think
it's a lot simpler to explain and a lot harder to break in future if we
just say that the names have to be valid identifiers.
Hearing no further comments, I pushed the more restrictive version.
regards, tom lane