Thread: snprintf assert is broken by plpgsql #option dump

snprintf assert is broken by plpgsql #option dump

From
Pavel Stehule
Date:
Hi

Today I had broken plpgsql_check tests aganst PostgreSQL 12. After command

create or replace function ml_trg()
returns trigger as $$
#option dump
declare
begin
  if TG_OP = 'INSERT' then
    if NEW.status_from IS NULL then
      begin
        -- performance issue only
        select status into NEW.status_from
           from pa
          where pa_id = NEW.pa_id;
        -- nonexist target value
        select status into NEW.status_from_xxx
           from pa
          where pa_id = NEW.pa_id;
      exception
        when DATA_EXCEPTION then
          new.status_from := 'DE';
      end;
    end if;
  end if;
  if TG_OP = 'DELETE' then return OLD; else return NEW; end if;
exception
  when OTHERS then
    NULL;
    if TG_OP = 'DELETE' then return OLD; else return NEW; end if;
end;
$$ language plpgsql;

I got backtrace:
Program received signal SIGABRT, Aborted.
0x00007f2c140e653f in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00007f2c140e653f in raise () from /lib64/libc.so.6
#1  0x00007f2c140d0895 in abort () from /lib64/libc.so.6
#2  0x00000000008b7903 in ExceptionalCondition (conditionName=conditionName@entry=0xb00d3b "!(strvalue != ((void *)0))",
    errorType=errorType@entry=0x906034 "FailedAssertion", fileName=fileName@entry=0xb00d30 "snprintf.c",
    lineNumber=lineNumber@entry=674) at assert.c:54
#3  0x00000000008ff368 in dopr (target=target@entry=0x7fff4ff6aad0, format=0x7f2bfe4b0de3 " fields",
    format@entry=0x7f2bfe4b0dda "ROW %-16s fields", args=args@entry=0x7fff4ff6ab18) at snprintf.c:674
#4  0x00000000008ff6b7 in pg_vfprintf (stream=<optimized out>, fmt=fmt@entry=0x7f2bfe4b0dda "ROW %-16s fields",
    args=args@entry=0x7fff4ff6ab18) at snprintf.c:261
#5  0x00000000008ff7ff in pg_printf (fmt=fmt@entry=0x7f2bfe4b0dda "ROW %-16s fields") at snprintf.c:286
#6  0x00007f2bfe4a7c67 in plpgsql_dumptree (func=func@entry=0x26811e8) at pl_funcs.c:1676
#7  0x00007f2bfe49a136 in do_compile (fcinfo=fcinfo@entry=0x7fff4ff6afa0, procTup=procTup@entry=0x7f2bfe4ba100, function=0x26811e8,
    function@entry=0x0, hashkey=hashkey@entry=0x7fff4ff6ad20, forValidator=forValidator@entry=true) at pl_comp.c:794
#8  0x00007f2bfe49a27a in plpgsql_compile (fcinfo=fcinfo@entry=0x7fff4ff6afa0, forValidator=forValidator@entry=true) at pl_comp.c:224
#9  0x00007f2bfe497126 in plpgsql_validator (fcinfo=<optimized out>) at pl_handler.c:504
#10 0x00000000008c03df in OidFunctionCall1Coll (functionId=functionId@entry=13392, collation=collation@entry=0, arg1=arg1@entry=40960)
    at fmgr.c:1418
#11 0x0000000000563444 in ProcedureCreate (procedureName=<optimized out>, procNamespace=procNamespace@entry=2200,
    replace=<optimized out>, returnsSet=<optimized out>, returnType=<optimized out>, proowner=16384, languageObjectId=13393,
    languageValidator=13392,
    prosrc=0x264feb8 "\n#option

There are new assert

Assert(strvalue != NULL);

probably all refname usage inside plpgsql dump functions has problem with it.

I found two parts

diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index b93f866223..d97997c001 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -1519,7 +1519,7 @@ dump_execsql(PLpgSQL_stmt_execsql *stmt)
        dump_ind();
        printf("    INTO%s target = %d %s\n",
               stmt->strict ? " STRICT" : "",
-              stmt->target->dno, stmt->target->refname);
+              stmt->target->dno, stmt->target->refname ? stmt->target->refname : "null");
    }
    dump_indent -= 2;
 }
@@ -1673,7 +1673,7 @@ plpgsql_dumptree(PLpgSQL_function *func)
                    PLpgSQL_row *row = (PLpgSQL_row *) d;
                    int         i;
 
-                   printf("ROW %-16s fields", row->refname);
+                   printf("ROW %-16s fields", row->refname ? row->refname : "null");
                    for (i = 0; i < row->nfields; i++)
                    {
                        printf(" %s=var %d", row->fieldnames[i],

Regards

Pavel

Re: snprintf assert is broken by plpgsql #option dump

From
Tom Lane
Date:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> There are new assert
> Assert(strvalue != NULL);
> probably all refname usage inside plpgsql dump functions has problem with
> it.

This isn't so much a "new assert" as a modeling of the fact that some
printf implementations dump core on a null string pointer, and have done
so since the dawn of time.

Now that we only use snprintf.c in HEAD, it'd be possible to consider
modeling glibc's behavior instead, ie instead of the Assert do

    if (strvalue == NULL)
        strvalue = "(null)";

I do not think this would be a good idea though, at least not till all
release branches that *don't* always use snprintf.c are out of support.
Every assert failure that we find here is a live bug in the back
branches, even if it happens not to occur on $your-favorite-platform.

Even once that window elapses, I'd not be especially on board with
snprintf.c papering over such cases.  They're bugs really.

> I found two parts

Thanks for the report, will push something.

            regards, tom lane


Re: snprintf assert is broken by plpgsql #option dump

From
Tom Lane
Date:
I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> I found two parts

> Thanks for the report, will push something.

On closer look, I'm not sure that these are the only places that are
assuming that any PLpgSQL_variable struct has a refname.  What seems
like a safer answer is to make sure they all do, more or less as
attached.

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 721234d..59460d2 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** build_row_from_vars(PLpgSQL_variable **v
*** 1896,1901 ****
--- 1896,1903 ----

      row = palloc0(sizeof(PLpgSQL_row));
      row->dtype = PLPGSQL_DTYPE_ROW;
+     row->refname = "(unnamed row)";
+     row->lineno = -1;
      row->rowtupdesc = CreateTemplateTupleDesc(numvars, false);
      row->nfields = numvars;
      row->fieldnames = palloc(numvars * sizeof(char *));
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 574234d..4552638 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_call(PLpgSQL_execstate *estate
*** 2205,2210 ****
--- 2205,2211 ----

              row = palloc0(sizeof(*row));
              row->dtype = PLPGSQL_DTYPE_ROW;
+             row->refname = "(unnamed row)";
              row->lineno = -1;
              row->varnos = palloc(sizeof(int) * FUNC_MAX_ARGS);

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index b59869a..68e399f 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** decl_cursor_args :
*** 613,618 ****
--- 613,619 ----

                          new = palloc0(sizeof(PLpgSQL_row));
                          new->dtype = PLPGSQL_DTYPE_ROW;
+                         new->refname = "(unnamed row)";
                          new->lineno = plpgsql_location_to_lineno(@1);
                          new->rowtupdesc = NULL;
                          new->nfields = list_length($2);
*************** read_into_scalar_list(char *initial_name
*** 3526,3531 ****
--- 3527,3533 ----

      row = palloc0(sizeof(PLpgSQL_row));
      row->dtype = PLPGSQL_DTYPE_ROW;
+     row->refname = "(unnamed row)";
      row->lineno = plpgsql_location_to_lineno(initial_location);
      row->rowtupdesc = NULL;
      row->nfields = nfields;
*************** make_scalar_list1(char *initial_name,
*** 3560,3565 ****
--- 3562,3568 ----

      row = palloc0(sizeof(PLpgSQL_row));
      row->dtype = PLPGSQL_DTYPE_ROW;
+     row->refname = "(unnamed row)";
      row->lineno = lineno;
      row->rowtupdesc = NULL;
      row->nfields = 1;
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4a4c7cb..f6c35a5 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_var
*** 326,332 ****
   * Note that there's no way to name the row as such from PL/pgSQL code,
   * so many functions don't need to support these.
   *
!  * refname, isconst, notnull, and default_val are unsupported (and hence
   * always zero/null) for a row.  The member variables of a row should have
   * been checked to be writable at compile time, so isconst is correctly set
   * to false.  notnull and default_val aren't applicable.
--- 326,337 ----
   * Note that there's no way to name the row as such from PL/pgSQL code,
   * so many functions don't need to support these.
   *
!  * That also means that there's no real name for the row variable, so we
!  * conventionally set refname to "(unnamed row)".  We could leave it NULL,
!  * but it's too convenient to be able to assume that refname is valid in
!  * all variants of PLpgSQL_variable.
!  *
!  * isconst, notnull, and default_val are unsupported (and hence
   * always zero/null) for a row.  The member variables of a row should have
   * been checked to be writable at compile time, so isconst is correctly set
   * to false.  notnull and default_val aren't applicable.

Re: snprintf assert is broken by plpgsql #option dump

From
Pavel Stehule
Date:


čt 4. 10. 2018 v 23:57 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> I found two parts

> Thanks for the report, will push something.

On closer look, I'm not sure that these are the only places that are
assuming that any PLpgSQL_variable struct has a refname.  What seems
like a safer answer is to make sure they all do, more or less as
attached.
 

+1

Pavel
 
                        regards, tom lane