| From: | Mario González Troncoso <gonzalemario(at)gmail(dot)com> |
|---|---|
| To: | li carol <carol(dot)li2025(at)outlook(dot)com> |
| Cc: | Bryan Green <dbryan(dot)green(at)gmail(dot)com>, Quan Zongliang <quanzongliang(at)yeah(dot)net>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Add pg_get_role_ddl() functions for role recreation |
| Date: | 2025-11-07 17:46:08 |
| Message-ID: | CAFsReFWO1f5coPu8UQBq9HR7KFWo5+dFmiU+pH8ZRVoSQk95uw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, 7 Nov 2025 at 02:43, li carol <carol(dot)li2025(at)outlook(dot)com> wrote:
>
> Hello Bryan,
>
> I reviewed your patch and found one potential issue, please check it.
> In pg_get_role_ddl_internal, the variable rolname is assigned from NameStr(roleform->rolname) (line 588), which means it points directly into the tuple returned from pg_authid. After constructing the initial CREATE ROLE statement, the code calls ReleaseSysCache(tuple); (line 665), so the memory holding that NameData now belongs to the cache again. However, the function continues to use rolname when building the subsequent ALTER ROLE statements (lines 756–765). Because the tuple has already been released, rolname is a dangling pointer and we risk reading garbage or crashing later. To fix this, copy the role name before releasing the syscache, e.g. rolname = pstrdup(NameStr(roleform->rolname));, and free it at the end.
>
Good catch, I didn't know NameStr returned a pointer, for some reason
I've assumed I was working with a copy. Attaching the patch with the
changes: (also I added you in "Reviewed-by")
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 584438d05ad..41db9f10f5d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -585,7 +585,7 @@ pg_get_role_ddl_internal(Oid roleid)
return NIL;
roleform = (Form_pg_authid) GETSTRUCT(tuple);
- rolname = NameStr(roleform->rolname);
+ rolname = pstrdup(NameStr(roleform->rolname));
/*
* We don't support generating DDL for system roles. The primary reason
@@ -777,6 +777,7 @@ pg_get_role_ddl_internal(Oid roleid)
table_close(rel, AccessShareLock);
pfree(buf.data);
+ pfree(rolname);
return statements;
https://cirrus-ci.com/build/4813271540170752
> BR,
> Yuan Li (Carol)
>
>
[...]
> >>
> >> Co-authored-by: Mario Gonzalez and Bryan Green.
> >>
> >> Comments?
> >>
> >> BG
> >
> The rebased patch is attached.
>
> Thanks,
>
> --
> Bryan Green
> EDB: https://www.enterprisedb.com
--
Mario Gonzalez
EDB: https://www.enterprisedb.com
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Add-functions-to-generate-DDL-for-recreating-roles.patch | text/x-patch | 23.6 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Joel Jacobson | 2025-11-07 18:59:41 | Re: Optimize LISTEN/NOTIFY |
| Previous Message | Fabrice Chapuis | 2025-11-07 17:34:33 | Re: Issue with logical replication slot during switchover |