Обсуждение: [PATCH][BUG FIX] Unsafe access pointers.

Hi,
Last time, I promise.

It's probably not happening, but it can happen, I think.

Best regards.
Ranier Vilela

--- \dll\postgresql-12.0\a\backend\access\brin\brin_validate.c    Mon Sep 30 17:06:55 2019
+++ brin_validate.c    Fri Nov 15 08:14:58 2019
@@ -57,8 +57,10 @@

     /* Fetch opclass information */
     classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclassoid));
-    if (!HeapTupleIsValid(classtup))
+    if (!HeapTupleIsValid(classtup)) {
         elog(ERROR, "cache lookup failed for operator class %u", opclassoid);
+        return false;
+    }
     classform = (Form_pg_opclass) GETSTRUCT(classtup);

     opfamilyoid = classform->opcfamily;
@@ -67,8 +69,11 @@

     /* Fetch opfamily information */
     familytup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(opfamilyoid));
-    if (!HeapTupleIsValid(familytup))
+    if (!HeapTupleIsValid(familytup)) {
         elog(ERROR, "cache lookup failed for operator family %u", opfamilyoid);
+        ReleaseSysCache(classtup);
+        return false;
+    }
     familyform = (Form_pg_opfamily) GETSTRUCT(familytup);

     opfamilyname = NameStr(familyform->opfname);

> On 15 Nov 2019, at 12:25, Ranier Vilela <ranier_gyn@hotmail.com> wrote:

> It's probably not happening, but it can happen, I think.

I don't think it can, given how elog() works.

> -    if (!HeapTupleIsValid(classtup))
> +    if (!HeapTupleIsValid(classtup)) {
>         elog(ERROR, "cache lookup failed for operator class %u", opclassoid);
> +        return false;

elog or ereport with a severity of ERROR or higher will never return.

> -    if (!HeapTupleIsValid(familytup))
> +    if (!HeapTupleIsValid(familytup)) {
>         elog(ERROR, "cache lookup failed for operator family %u", opfamilyoid);
> +        ReleaseSysCache(classtup);
> +        return false;
> +    }

Not only will elog(ERROR ..) not return to run this, the errorhandling
machinery will automatically release resources and clean up.

cheers ./daniel

Hi,
Thank you for the explanation.

Best regards.
Ranier Vilela
________________________________________
De: Daniel Gustafsson <daniel@yesql.se>
Enviado: sexta-feira, 15 de novembro de 2019 11:58
Para: Ranier Vilela
Cc: pgsql-hackers@lists.postgresql.org
Assunto: Re: [PATCH][BUG FIX] Unsafe access pointers.

> On 15 Nov 2019, at 12:25, Ranier Vilela <ranier_gyn@hotmail.com> wrote:

> It's probably not happening, but it can happen, I think.

I don't think it can, given how elog() works.

> -     if (!HeapTupleIsValid(classtup))
> +     if (!HeapTupleIsValid(classtup)) {
>               elog(ERROR, "cache lookup failed for operator class %u", opclassoid);
> +        return false;

elog or ereport with a severity of ERROR or higher will never return.

> -     if (!HeapTupleIsValid(familytup))
> +     if (!HeapTupleIsValid(familytup)) {
>               elog(ERROR, "cache lookup failed for operator family %u", opfamilyoid);
> +         ReleaseSysCache(classtup);
> +        return false;
> +    }

Not only will elog(ERROR ..) not return to run this, the errorhandling
machinery will automatically release resources and clean up.

cheers ./daniel

On 2019-Nov-15, Ranier Vilela wrote:

> Hi,
> Last time, I promise.
> 
> It's probably not happening, but it can happen, I think.

This patch assumes that anything can happen after elog(ERROR).  That's
wrong -- under ERROR or higher, elog() (as well as ereport) never
returns to the caller.  If this was possible, there would be thousands
of places that would need to be patched, all over the server code.  But
it's not.

>      /* Fetch opclass information */
>      classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclassoid));
> -    if (!HeapTupleIsValid(classtup))
> +    if (!HeapTupleIsValid(classtup)) {
>          elog(ERROR, "cache lookup failed for operator class %u", opclassoid);
> +        return false;
> +    }


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services