Em qua., 27 de mar. de 2024 às 14:35, Nathan Bossart < nathandboss...@gmail.com> escreveu:
> 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. > Sorry for exceeding. > > - 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. > Ok. If admin_role is NOT NULL, so *admin_role is InvalidOid, by initialization in the head of function. I think that a cheap test *admin_role == InvalidOid, is enough? What do you think? v1 attached. best regards, Ranier Vilela
v1-avoid-dereference-a-null-pointer-acl.patch
Description: Binary data
