Обсуждение: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

The following bug has been logged on the website:

Bug reference:      17909
Logged by:          Song Hongyu
Email address:      hysong0101@163.com
PostgreSQL version: 15.0
Operating system:   centos7
Description:

When we CREATE SCHEMA AUTHORIZATION rolname CREATE TABLE/SEQUENCE/VIEW
sch.obj and sch is not NULL,
the database will coredump.

The reason is in parse_utilcmd.c we will check whether schemaName and
rolname are same or not. The pointer is not checked in strcmp there, so the
database coredump is caused.

On Thu, Apr 27, 2023 at 03:44:04AM +0000, PG Bug reporting form wrote:
> When we CREATE SCHEMA AUTHORIZATION rolname CREATE TABLE/SEQUENCE/VIEW
> sch.obj and sch is not NULL,
> the database will coredump.

It took me a couple of minutes to get what you meant here.  The point
is that schema-qualifying any of the object specified after the CREATE
SCHEMA with a schema name different than the rolname would cause a
crash, when no schema is directly given.  We should fail with the same
error than when a schema is specified, as of, except that the rolename
needs to be specified:
=# create schema popo authorization postgres create table lala.aa (a int);
ERROR:  42P15: CREATE specifies a schema (lala) different from the one being created (popo)

That seems quite old, at quick glance (v11 fails), so this needs to be
fixed all the way down.  Will fix, nice catch!
--
Michael

On Thu, Apr 27, 2023 at 04:59:13PM +0800, Richard Guo wrote:
> In this case the CreateSchemaStmtContext.schemaname is NULL since it is
> not explicitly specified, while the schemaname in the schema element is
> not NULL as it is specified, and setSchemaName cannot copy with such
> situation.  Maybe we should check against RoleSpec.rolename in this case
> since that is also the schema's name?

In this case, it is cleaner to just set the schema name in
CreateSchemaStmtContext.schemaname to the role in the RoleSpec if there
is no schema set in the query, because the schema name will have the
same name as the role.  That also makes the handling of each element
in schemaElts simpler.

The regression tests cruelly lacks of checks here.  This is not a
pattern of CREATE SCHEMA known a lot, but we should do better.
--
Michael

On Thu, Apr 27, 2023 at 06:28:28PM +0800, Richard Guo wrote:
> I noticed that in CreateSchemaCommand there is logic that fills schema
> name with the role name if it is not specified.  Do you think we can
> save the new-filled schema name into CreateSchemaStmt.schemaname there?

It's actually much trickier than that, on second look, as a RoleSpec
is embedded in this portion of a query because we need to support
CURRENT_ROLE, CURRENT_SESSION and SESSION_USER.  For example, this
query assigns neither role name nor schema name we could rely on at
transformation for the objects:
create schema authorization current_role create table aa (a int);

Anyway, semantically, something could go very wrong if we decide to
enforce a schema name for the objects based on the RoleSpec at the
time of transformation, because we may finish by executing the CREATE
SCHEMA command under an entirely different context than what we could
assign.  So, I think that we should do the following in the
transformation path if an object is schema-qualified:
- Fail immediately if there is no schema and no role name at hand,
just give up.  This needs a new error message, say:
"CREATE specifies a schema (%object_schema) without providing a schema
name."
- If there is a role name, aka RoleSpec points to a ROLESPEC_CSTRING,
use it as a comparison with the objects schema-qualified.  Note that
there is no case for public, because we would fail on
get_rolespec_oid() when the schema is created.

There is also a very fancy case, if "foo" matches to the role that
would be assigned by GetUserIdAndSecContext() when executing the
schema command:
create schema authorization session_role create table foo.aa (a int);

One could say that this should work, and my proposal would cause an
error to make the query more predictible at an earlier step.  IMO, I
think that this is just saner.  And this case crashes today like the
others.

Any thoughts or objections about doing that?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> It's actually much trickier than that, on second look, as a RoleSpec
> is embedded in this portion of a query because we need to support
> CURRENT_ROLE, CURRENT_SESSION and SESSION_USER.  For example, this
> query assigns neither role name nor schema name we could rely on at
> transformation for the objects:
> create schema authorization current_role create table aa (a int);

Seems like the answer needs to involve postponing examination of
the contained DDL until we've figured out what the new schema's
name is going to be.

            regards, tom lane

On Thu, Apr 27, 2023 at 07:48:49PM -0400, Tom Lane wrote:
> Seems like the answer needs to involve postponing examination of
> the contained DDL until we've figured out what the new schema's
> name is going to be.

Actually, wait a min..  The transformation of the objects is applied
during the execution of the CREATE SCHEMA command, but nowhere else,
so if you give to transformCreateSchemaStmt() the name of the expected
schema rather than rely on the schema name from the query this should
work OK.
--
Michael

On Fri, Apr 28, 2023 at 08:56:27AM +0900, Michael Paquier wrote:
> Actually, wait a min..  The transformation of the objects is applied
> during the execution of the CREATE SCHEMA command, but nowhere else,
> so if you give to transformCreateSchemaStmt() the name of the expected
> schema rather than rely on the schema name from the query this should
> work OK.

So, the source of my confusion is the design currently used for
transformCreateSchemaStmt():
- The schema name is extracted from the query itself, but we have a
schema compiled from a role specification, depending on how the
beginning of CreateSchemaCommand() feels it.
- This routine includes a reference to the role specification in the
context, but makes no use of it.  Perhaps somebody would be
interested in this information in the future if the query support is
improved, but one could also be tempted to feed the schema name based
on the RoleSpec, which I'd rather avoid for the moment.

Attached is what I am finishing with, where I have reworked
transformCreateSchemaStmt() so as it uses in input the list of
elements from CREATE SCHEMA and the schema name computed depending on
the security context, documenting requirements on the way (note the
extra unconstify for the RangeVars' schemas).  I have added a couple
of regression tests for all the object types that have schema
qualication checks, mixed with role specs and schema names.

Thoughts, comments or objections?
--
Michael

On Fri, Apr 28, 2023 at 11:29:11AM +0800, Richard Guo wrote:
> BTW, the comment states that CreateSchemaStmtContext.stmtType is "CREATE
> SCHEMA" or "ALTER SCHEMA".   But it seems that there is no chance to set
> it to "ALTER SCHEMA".  So should we update that comment, or go even
> further to remove CreateSchemaStmtContext.stmtType since it is not used?

Indeed.  I'd be OK with adjusting the comment, without removing
stmtType to keep some consistency with CreateStmt, and it could be
useful for debugging, perhaps..  (See 46379d6 as one origin point).
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Apr 28, 2023 at 11:29:11AM +0800, Richard Guo wrote:
>> BTW, the comment states that CreateSchemaStmtContext.stmtType is "CREATE
>> SCHEMA" or "ALTER SCHEMA".   But it seems that there is no chance to set
>> it to "ALTER SCHEMA".  So should we update that comment, or go even
>> further to remove CreateSchemaStmtContext.stmtType since it is not used?

> Indeed.  I'd be OK with adjusting the comment, without removing
> stmtType to keep some consistency with CreateStmt, and it could be
> useful for debugging, perhaps..  (See 46379d6 as one origin point).

I'd be okay with just dropping that field.  It seems to be much
older than 46379d6, and if it ever had any real use it doesn't now.
(There's no ALTER SCHEMA in the SQL spec at all, let alone one that
has some overlap with CREATE SCHEMA options, so I don't foresee a
future use either.)

            regards, tom lane

On Fri, Apr 28, 2023 at 12:53:39AM -0400, Tom Lane wrote:
> I'd be okay with just dropping that field.  It seems to be much
> older than 46379d6, and if it ever had any real use it doesn't now.
> (There's no ALTER SCHEMA in the SQL spec at all, let alone one that
> has some overlap with CREATE SCHEMA options, so I don't foresee a
> future use either.)

WFM as well.
--
Michael

On Fri, Apr 28, 2023 at 02:07:36PM +0900, Michael Paquier wrote:
> WFM as well.

Note that this has been applied as of 4dadd66 down to v11.  Thanks for
the report, the reviews and the discussion!
--
Michael