Fix crash in supautils_executor_start by pgnickb · Pull Request #190 · supabase/supautils

pgnickb · 2026-04-28T08:54:53Z

supautils_executor_start makes some assumptions about the ERROR it is intercepting. Following are addressed:

Fixed cases where the target object isn't a relation
Fixed possible cases of trying to free a NULL variable
Fixed possible leak of an old hint (if there were one)

This fixes existing bugs. However it doesn't add the missing functionality for the cases when privileges are missing for functions, schemas etc.

I also want to point out that executor is a very low level API and hooking into it merely to slightly improve user experience seems like an overkill to me. We could consider writing a standalone function to the effect of check_permissions(objclassid, objid, rolename) that would perform the same checks as the hook, but won't reside in a critical codepath so any potential bugs won't crash the database.

supautils_executor_start makes some assumptions about the ERROR it is intercepting. Following are addressed: - Fixed cases where the target object isn't a relation - Fixed possible cases of trying to free a NULL variable - Fixed possible leak of an old hint (if there were one)

coveralls · 2026-04-28T08:57:06Z

Coverage Report for CI Build 25060370145

Coverage increased (+0.05%) to 88.79%

Details

Coverage increased (+0.05%) from the base build.
Patch coverage: 15 of 15 lines across 1 file are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1240
Covered Lines:	1101
Line Coverage:	88.79%
Coverage Strength:	32.18 hits per line

💛 - Coveralls

Copilot

Pull request overview

This PR adjusts supautils_executor_start’s permission-hint enrichment to be safer when constructing edata->hint, aiming to avoid crashes and memory issues while intercepting ERRCODE_INSUFFICIENT_PRIVILEGE.

Changes:

Adds a NULL check around relation-name handling before composing a GRANT hint.
Frees any existing edata->hint before replacing it to avoid leaking the old hint.
Adds/adjusts pfree cleanup paths to avoid freeing NULL and to release intermediate strings.

Sign in to view

        char *schema  = get_namespace_name(get_rel_namespace(missing.relid));
        char *relname = get_rel_name(missing.relid);
-        char *qualified_rel_name = quote_qualified_identifier(schema, relname);
-        char *quoted_role_name   = quote_qualified_identifier(
-            NULL, GetUserNameFromId(current_role_oid, false));

-        edata->hint = psprintf("Grant the required privileges to the current "
-                               "role with: GRANT %s ON %s TO %s;",
-                               privileges_str->data, qualified_rel_name,
-                               quoted_role_name);
+        if (relname != NULL) {
+          char *qualified_rel_name =


Indeed asserts are not a proper way to protect against InvalidOid or 0 ACLs. Use proper runtime guards instead.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

za-arthur · 2026-04-28T11:06:21Z

Sign in to view

+              pfree(username);
+              pfree(qualified_rel_name);
+              pfree(quoted_role_name);


Copilot is wrong

Copilot · 2026-04-28T09:37:57Z

      if (edata->sqlerrcode == ERRCODE_INSUFFICIENT_PRIVILEGE) {
        const Oid current_role_oid = GetUserId();

-        // edata->table_name is always NULL for some reason, so we need to find
-        // the relid (included in missing_perm)
        missing_perm missing =
            find_missing_perm(queryDesc->plannedstmt, current_role_oid);

-        // there must be some privilege missing given there was a
-        // ERRCODE_INSUFFICIENT_PRIVILEGE
-        Assert(missing.acl != 0);
-        Assert(missing.relid != InvalidOid);
-
-        // ERRCODE_INSUFFICIENT_PRIVILEGE also includes TRUNCATE, REFERENCES and
-        // TRIGGER error privileges but these are not visible in this function
-        // (a ExecutorStart_hook) so we assert here that these are impossible as
-        // a missing privilege
-        Assert((missing.acl & (ACL_TRUNCATE | ACL_TRIGGER | ACL_REFERENCES)) ==
-               0);
-
-        StringInfo privileges_str = makeStringInfo();
-        build_privileges_string(privileges_str, missing.acl);
-
-        // the string of privileges has to be built
-        Assert(privileges_str->len > 0);
-
-        char *schema  = get_namespace_name(get_rel_namespace(missing.relid));
-        char *relname = get_rel_name(missing.relid);
-        char *qualified_rel_name = quote_qualified_identifier(schema, relname);
-        char *quoted_role_name   = quote_qualified_identifier(
-            NULL, GetUserNameFromId(current_role_oid, false));
-
-        edata->hint = psprintf("Grant the required privileges to the current "
-                               "role with: GRANT %s ON %s TO %s;",
-                               privileges_str->data, qualified_rel_name,
-                               quoted_role_name);
-
-        destroyStringInfo(privileges_str);
-        pfree(schema);
-        pfree(relname);
-        pfree(qualified_rel_name);
-        pfree(quoted_role_name);
+        if (missing.acl != 0 && OidIsValid(missing.relid) &&
+            (missing.acl & (ACL_TRUNCATE | ACL_TRIGGER | ACL_REFERENCES)) ==
+                0) {


This change is intended to avoid crashes when ERRCODE_INSUFFICIENT_PRIVILEGE is raised for non-relation objects (schemas/functions/etc). There are existing regression tests for permission hints (test/sql/permission_hints.sql); please add coverage for at least one non-relation insufficient-privilege error to ensure the hook no longer asserts/crashes and to pin the expected hint behavior (likely: no enhanced hint).

za-arthur · 2026-04-28T09:52:37Z

-        pfree(relname);
-        pfree(qualified_rel_name);
-        pfree(quoted_role_name);
+        if (missing.acl != 0 && OidIsValid(missing.relid) &&


Suggested change

if (missing.acl != 0 && OidIsValid(missing.relid) &&

if (missing.acl != ACL_NO_RIGHTS && OidIsValid(missing.relid) &&

za-arthur · 2026-04-28T11:00:15Z

+
+          if (privileges_str->len > 0) {
+            char *schema = get_namespace_name(get_rel_namespace(missing.relid));
+            char *relname = get_rel_name(missing.relid);


Can get_rel_name return NULL here if missing.relid is not InvalidOid? We check missing.relid above and if get_rel_name returns NULL then this is usually a cache error.

Good point. Now that we don't simply rely on asserts to check if relid is 0 it might be unnecessary. Before we did end up here with relid=0 and that was the main cause for the crash.

We should do this refactor now otherwise it will be harder to change later

if get_rel_name returns NULL then this is usually a cache error.

On second though, if what Artur mentions above is true, then maybe we should leave it as extra layer of defense.

za-arthur · 2026-04-28T11:06:21Z

+              pfree(username);
+              pfree(qualified_rel_name);
+              pfree(quoted_role_name);


Copilot is wrong

steve-chavez · 2026-04-28T14:34:16Z

This fixes existing bugs. However it doesn't add the missing functionality for the cases when privileges are missing for functions, schemas etc.

Yes, it was limited on purpose and it doesn't work for views too #182. Will put a note there.

I also want to point out that executor is a very low level API and hooking into it merely to slightly improve user experience seems like an overkill to me. We could consider writing a standalone function to the effect of check_permissions(objclassid, objid, rolename) that would perform the same checks as the hook, but won't reside in a critical codepath so any potential bugs won't crash the database.

Where would check_permissions hook into? executor hook looked like the only choice when I added this.

Signed-off-by: Bobbie Soedirgo <bobbie@soedirgo.dev>

pgnickb · 2026-04-28T15:01:55Z

@steve-chavez

I also want to point out that executor is a very low level API and hooking into it merely to slightly improve user experience seems like an overkill to me. We could consider writing a standalone function to the effect of check_permissions(objclassid, objid, rolename) that would perform the same checks as the hook, but won't reside in a critical codepath so any potential bugs won't crash the database.

Where would check_permissions hook into? executor hook looked like the only choice when I added this.

That's the point: the user would get an error and then call the function manually. No need to hook into anything. That's just asking for problems at this level. IMO error handling is a critical enough path that we should be very careful what we change there.

steve-chavez · 2026-04-28T15:07:48Z

That's the point: the user would get an error and then call the function manually. No need to hook into anything.

@pgnickb When I looked at that option there was no way to obtain which exact privileges were missing (this is lost after the executor stage) and this was a requirement.

pgnickb requested a review from Copilot April 28, 2026 08:55

Copilot started reviewing on behalf of pgnickb April 28, 2026 08:55 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

pgnickb added 2 commits April 28, 2026 11:22

Address copilot's concern

459c990

Indeed asserts are not a proper way to protect against InvalidOid or 0 ACLs. Use proper runtime guards instead.

Make linter happy

5fcd86c

pgnickb requested a review from Copilot April 28, 2026 09:32

Copilot started reviewing on behalf of pgnickb April 28, 2026 09:32 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

pgnickb marked this pull request as ready for review April 28, 2026 10:12

za-arthur reviewed Apr 28, 2026

View reviewed changes

pgnickb added 2 commits April 28, 2026 13:55

Add regression test

116bf6a

Remove unnecessary pfree() calls

4dc1621

soedirgo approved these changes Apr 28, 2026

View reviewed changes

steve-chavez reviewed Apr 28, 2026

View reviewed changes

Comment thread src/supautils.c

steve-chavez reviewed Apr 28, 2026

View reviewed changes

Comment thread src/supautils.c

steve-chavez mentioned this pull request Apr 28, 2026

Enhanced hints don't work for pg 18 views #182

Open

steve-chavez approved these changes Apr 28, 2026

View reviewed changes

soedirgo added 2 commits April 28, 2026 22:56

chore: style

066ed6d

Signed-off-by: Bobbie Soedirgo <bobbie@soedirgo.dev>

chore: bump version

a4c0818

Signed-off-by: Bobbie Soedirgo <bobbie@soedirgo.dev>

za-arthur approved these changes Apr 28, 2026

View reviewed changes

soedirgo mentioned this pull request Apr 28, 2026

chore: bump module version #189

Closed

soedirgo merged commit 64792e1 into master Apr 28, 2026
12 checks passed

soedirgo deleted the nickb/fix_crash_in_supautils_executor_start branch April 28, 2026 15:07

steve-chavez mentioned this pull request May 15, 2026

Sanity tests using PostgreSQL test suite #101

Open

	if (missing.acl != 0 && OidIsValid(missing.relid) &&
	if (missing.acl != ACL_NO_RIGHTS && OidIsValid(missing.relid) &&

Uh oh!

Conversation

pgnickb commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 25060370145

Coverage increased (+0.05%) to 88.79%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

This comment was marked as low quality.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

This comment was marked as low quality.

Uh oh!

za-arthur Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

za-arthur Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

za-arthur Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

pgnickb Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

za-arthur Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steve-chavez commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgnickb commented Apr 28, 2026

Uh oh!

Uh oh!

steve-chavez commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pgnickb commented Apr 28, 2026 •

edited

Loading

coveralls commented Apr 28, 2026 •

edited

Loading

steve-chavez commented Apr 28, 2026 •

edited

Loading