Repository navigation
fix: guard role lookups against statements that name no role - #259
Conversation
|
Are there other null dereference issues outside of the reported |
No I checked at all places. I didn't find any new apart from this . There were couple of similar issues which we fixed earlier. Most of the changes in the code are contributed to hardening of the existing code. |
imor
left a comment
imor
left a comment
There was a problem hiding this comment.
Left just one comment, otherwise LGTM.
|
|
||
| reset role; | ||
| -- the session is still usable | ||
| select 1 as alive; |
There was a problem hiding this comment.
The ge16_role_spec_forms.sql and lt16_role_spec_forms.sql are duplicates of each other. If these were duplicated because their corresponding .out files were duplicate then an alternative is to have those differences resolved in the .out.in files via templating which will allow use to remove the .sql duplicate files. See the event_triggers.out.it file for how it's done there.
| /* | ||
| * CREATE EXTENSION <extension> | ||
| */ | ||
| static bool create_extension(CreateExtensionStmt *stmt, |
There was a problem hiding this comment.
Any reason for this change, passing CreateExtensionStmt* before and now *extname or **options doesn't change anything - same question for other function changes in PR that was previously passed object and now we pass specific fields that are needed in function. I would expect that function checks nulls/whatever and decided result. I understand that is subject of PR but just wondering why it's needed
|
|
||
| switch (nodeTag(stmt)) { | ||
| case T_AlterRoleStmt: { | ||
| alter_role_cmd cmd = normalize_alter_role((AlterRoleStmt *)stmt); |
There was a problem hiding this comment.
Sam question as previous comment, now we introduce this normalize* function that are not used anywhere except alter_role (this examples).
My 2 cents is that this PR seems very big for fixing a single segfault. For reference I fixed that segfault with like 20 LOC in my closed attempt. Perhaps this PR is better even with the larger diffs, I would need to invest more time in researching this to know for sure for myself. If you guys think this is the way forward I wont hold up this PR if I'm the only one who thinks this way. |
|
I think the same as above. Also a bit confused why the |
91b258c to
e46213a
Compare
utkarash2991
changed the title
e46213a to
9122ad7
Compare
|
@steve-chavez @imor @AndrewJackson2020 @mkaruza Thanks for the review and comments on the Pr. I have simplified the Pr just to add needed checks now . Can you please give it a look. |
| /* | ||
| * The raw role lookups require a RoleSpec that names a role; ALTER ROLE ALL | ||
| * has none and PUBLIC is not one. Every read goes through | ||
| * rolespec_name_or_null() and role_exists() in utils.c, the only file that may | ||
| * define SUPAUTILS_ALLOW_RAW_ROLESPEC. Anywhere else these names are blocked | ||
| * at compile time. | ||
| */ | ||
| #ifndef SUPAUTILS_ALLOW_RAW_ROLESPEC | ||
| # pragma GCC poison get_rolespec_name get_rolespec_tuple get_rolespec_oid get_role_oid | ||
| #endif |
There was a problem hiding this comment.
This is a really cool way to prevent these unsafe calls.
I think we could have rolespec_name_or_null on pg_prelude and remove the #ifndef SUPAUTILS_ALLOW_RAW_ROLESPEC as a further/later improvement.
There was a problem hiding this comment.
Sure. Taking care of the follow up in : #268
9122ad7 to
13e613b
Compare
ALTER ROLE ALL carries no RoleSpec and PUBLIC is not a role, but the role handlers passed both straight to the core lookups. Read every RoleSpec through rolespec_name_or_null() and block the raw lookups at compile time outside utils.c so the same mistake cannot build again.
13e613b to
a7b2715
Compare
ALTER ROLE ALL has no role spec and PUBLIC is not a role, but the role handlers were passing both straight to the core lookups. Added a small helper that returns NULL for these cases and used it at every place a role spec is read. The raw lookups are blocked at compile time outside utils.c so the same mistake cannot build again.
Tests for all the role spec forms are in #260.
Verified on PG 14, 15, 17 and 19.