On Wed, Mar 27, 2024 at 01:47:38PM -0300, Ranier Vilela wrote: > Em qua., 27 de mar. de 2024 às 13:41, Nathan Bossart < > nathandboss...@gmail.com> escreveu: >> On Wed, Mar 27, 2024 at 01:21:23PM -0300, Ranier Vilela wrote: >> > I think that left an oversight in a commit d365ae7 >> > < >> https://github.com/postgres/postgres/commit/d365ae705409f5d9c81da4b668f59c3598feb512 >> > >> > If the admin_role is a NULL pointer, so, can be dereferenced >> > in the main loop of the function roles_is_member_of and >> > worst, IMO, can be destroying aleatory memory? >> > >> > First, is a better shortcut test to check if admin_role is NOT NULL. >> > Second, !OidIsValid(*admin_role), It doesn't seem necessary anymore. >> > >> > Or am I losing something? >> >> If admin_role is NULL, then admin_of is expected to be set to InvalidOid. >> See the assertion at the top of the function. AFAICT the code that >> dereferences admin_role short-circuits if admin_of is invalid. >> > These conditions seem a little fragile and confusing to me. > When a simple test, it protects the pointer and avoids a series of tests, > which are unnecessary if the pointer is invalid.
Maybe. But that doesn't seem like an oversight in commit d365ae7. - if (otherid == admin_of && form->admin_option && - OidIsValid(admin_of) && !OidIsValid(*admin_role)) + if (admin_role != NULL && otherid == admin_of && form->admin_option && + OidIsValid(admin_of)) *admin_role = memberid; I'm not following why it's safe to remove the !OidIsValid(*admin_role) check here. We don't want to overwrite a previously-set value of *admin_role, as per the comment above roles_is_member_of(): * If admin_of is not InvalidOid, this function sets *admin_role, either * to the OID of the first role in the result list that directly possesses * ADMIN OPTION on the role corresponding to admin_of, or to InvalidOid if * there is no such role. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com