From: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c) |
Date: | 2024-05-24 06:28:51 |
Message-ID: | CAExHW5udy9VYw-Abrx37DmEaA+XNy7LDzgeH-kx=MO5AqCqgzg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, May 24, 2024 at 11:03 AM Michael Paquier <michael(at)paquier(dot)xyz>
wrote:
> On Thu, May 23, 2024 at 08:54:12AM -0300, Ranier Vilela wrote:
> > All calls to functions like SearchSysCacheAttName, in the whole codebase,
> > checks if returns are valid.
> > It must be for a very strong reason, such a style.
>
> Usually good practice, as I've outlined once upthread, because we do
> expect the attributes to exist in this case. Or if you want, an error
> is better than a crash if a concurrent path causes this area to lead
> to inconsistent lookups, which is something I've seen in the past
> while hacking on my own stuff, or just fix other things causing
> syscache lookup inconsistencies. You'd be surprised to hear that
> dropped attributes being mishandled is not that uncommon, especially
> in out-of-core code, as one example. FWIW, I don't see much a point
> in using ereport(), the two checks ought to be elog()s pointing to an
> internal error as these two errors should never happen. Still, it is
> a good idea to check that they never happen: aka an internal
> error state is better than a crash if a problem arises.
>
> > So, v3, implements it this way.
>
> I don't understand the point behind the open/close of attrelation,
> TBH. That's not needed.
>
> Except fot these two points, this is just moving the calls to make
> sure that we have valid tuples from the syscache, which is a better
> practice. 509199587df7 is recent enough that this should be fixed now
> rather than later.
>
If we are looking for avoiding a segfault and get a message which helps
debugging, using get_attname and get_attnum might be better options.
get_attname throws an error. get_attnum doesn't throw an error and returns
InvalidAttnum which won't return any valid identity sequence, and thus
return a NIL sequence list which is handled in that function already. Using
these two functions will avoid the clutter as well as segfault. If that's
acceptable, I will provide a patch.
--
Best Wishes,
Ashutosh Bapat
From | Date | Subject | |
---|---|---|---|
Next Message | Mats Kindahl | 2024-05-24 06:36:32 | Re: Table AM Interface Enhancements |
Previous Message | Long Song | 2024-05-24 06:02:52 | Re:about cross-compiling issue |