Postgres Pro
Downloads
Home   >   mailing lists

Thread: [PATCH] Tiny optmization.

[PATCH] Tiny optmization.

From
Ranier Vilela
Date:
Hi,
Maybe it doesn't matter, but, I think it's worth discussing.
The expression "if(pstate)" is redundant or is possible null dereference.

Best regards.
Ranier Vilela

--- \dll\postgresql-12.0\a\backend\commands\copy.c    Mon Sep 30 17:06:55 2019
+++ copy.c    Fri Nov 22 18:33:05 2019
@@ -3426,8 +3426,7 @@
     cstate->raw_buf_index = cstate->raw_buf_len = 0;

     /* Assign range table, we'll need it in CopyFrom. */
-    if (pstate)
-        cstate->range_table = pstate->p_rtable;
+    cstate->range_table = pstate->p_rtable;

     tupDesc = RelationGetDescr(cstate->rel);
     num_phys_attrs = tupDesc->natts;
Attachment

Re: [PATCH] Tiny optmization.

From
Tomas Vondra
Date:
On Fri, Nov 22, 2019 at 09:41:44PM +0000, Ranier Vilela wrote:
>Hi,
>Maybe it doesn't matter, but, I think it's worth discussing.
>The expression "if(pstate)" is redundant or is possible null dereference.
>

Eh? Redundant with what? Why would it be a null dereference? It's a
parameter passed from outside, and we're not checking it before. And
the if condition is there exactly to prevent null dereference.

It's generally a good idea to inspect existing callers of the modified
function and try running tests before submitting a patch. In this case
there's a BeginCopyFrom() call in contrib/file_fdw, passing NULL as the
first parameter, and if you run `make check` for that module it falls
flat on it's face due to a segfault.

regards

>
>--- \dll\postgresql-12.0\a\backend\commands\copy.c    Mon Sep 30 17:06:55 2019
>+++ copy.c    Fri Nov 22 18:33:05 2019
>@@ -3426,8 +3426,7 @@
>     cstate->raw_buf_index = cstate->raw_buf_len = 0;
>
>     /* Assign range table, we'll need it in CopyFrom. */
>-    if (pstate)
>-        cstate->range_table = pstate->p_rtable;
>+    cstate->range_table = pstate->p_rtable;
>
>     tupDesc = RelationGetDescr(cstate->rel);
>     num_phys_attrs = tupDesc->natts;



-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: [PATCH] Tiny optmization.

From
Ranier Vilela
Date:
Hi,
Redudant because he it's been dereferenced here:

line 3410:
        cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);

Best regards.
Ranier Vilela

________________________________________
De: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Enviado: sexta-feira, 22 de novembro de 2019 22:05
Para: Ranier Vilela
Cc: pgsql-hackers@postgresql.org
Assunto: Re: [PATCH] Tiny optmization.

On Fri, Nov 22, 2019 at 09:41:44PM +0000, Ranier Vilela wrote:
>Hi,
>Maybe it doesn't matter, but, I think it's worth discussing.
>The expression "if(pstate)" is redundant or is possible null dereference.
>

Eh? Redundant with what? Why would it be a null dereference? It's a
parameter passed from outside, and we're not checking it before. And
the if condition is there exactly to prevent null dereference.

It's generally a good idea to inspect existing callers of the modified
function and try running tests before submitting a patch. In this case
there's a BeginCopyFrom() call in contrib/file_fdw, passing NULL as the
first parameter, and if you run `make check` for that module it falls
flat on it's face due to a segfault.

regards

>
>--- \dll\postgresql-12.0\a\backend\commands\copy.c     Mon Sep 30 17:06:55 2019
>+++ copy.c     Fri Nov 22 18:33:05 2019
>@@ -3426,8 +3426,7 @@
>       cstate->raw_buf_index = cstate->raw_buf_len = 0;
>
>       /* Assign range table, we'll need it in CopyFrom. */
>-      if (pstate)
>-              cstate->range_table = pstate->p_rtable;
>+      cstate->range_table = pstate->p_rtable;
>
>       tupDesc = RelationGetDescr(cstate->rel);
>       num_phys_attrs = tupDesc->natts;



--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Tiny optmization.

From
Tom Lane
Date:
Ranier Vilela <ranier_gyn@hotmail.com> writes:
> Redudant because he it's been dereferenced here:
> line 3410:
>         cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);

Not necessarily ... the rel!=NULL code path there doesn't touch pstate,
and that seems to be what contrib/file_fdw is relying on.

Arguably, the rel==NULL code path in BeginCopy should be prepared to
support pstate being null, too.  But what you proposed here is certainly
not OK.

            regards, tom lane

Re: [PATCH] Tiny optmization.

From
Tomas Vondra
Date:
On Fri, Nov 22, 2019 at 10:10:29PM +0000, Ranier Vilela wrote:
>Hi,
>Redudant because he it's been dereferenced here:
>
>line 3410:
>        cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);
>

There's no pstate dereference here. It just passed the value to
BeginCopy.

BTW please don't top post, reply inline. It's much easier to follow the
discussion.


>Best regards.
>Ranier Vilela
>
>________________________________________
>De: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>Enviado: sexta-feira, 22 de novembro de 2019 22:05
>Para: Ranier Vilela
>Cc: pgsql-hackers@postgresql.org
>Assunto: Re: [PATCH] Tiny optmization.
>
>On Fri, Nov 22, 2019 at 09:41:44PM +0000, Ranier Vilela wrote:
>>Hi,
>>Maybe it doesn't matter, but, I think it's worth discussing.
>>The expression "if(pstate)" is redundant or is possible null dereference.
>>
>
>Eh? Redundant with what? Why would it be a null dereference? It's a
>parameter passed from outside, and we're not checking it before. And
>the if condition is there exactly to prevent null dereference.
>
>It's generally a good idea to inspect existing callers of the modified
>function and try running tests before submitting a patch. In this case
>there's a BeginCopyFrom() call in contrib/file_fdw, passing NULL as the
>first parameter, and if you run `make check` for that module it falls
>flat on it's face due to a segfault.
>
>regards
>
>>
>>--- \dll\postgresql-12.0\a\backend\commands\copy.c     Mon Sep 30 17:06:55 2019
>>+++ copy.c     Fri Nov 22 18:33:05 2019
>>@@ -3426,8 +3426,7 @@
>>       cstate->raw_buf_index = cstate->raw_buf_len = 0;
>>
>>       /* Assign range table, we'll need it in CopyFrom. */
>>-      if (pstate)
>>-              cstate->range_table = pstate->p_rtable;
>>+      cstate->range_table = pstate->p_rtable;
>>
>>       tupDesc = RelationGetDescr(cstate->rel);
>>       num_phys_attrs = tupDesc->natts;
>
>
>
>--
>Tomas Vondra                  http://www.2ndQuadrant.com
>PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: [PATCH] Tiny optmization.

From
Ranier Vilela
Date:
Hi,
pstate is touched here:
a) BeginCopy line 1489:
    ProcessCopyOptions(pstate, cstate, is_from, options);
b) ProcessCopyOptions line 1137:

            if (format_specified)
                ereport(ERROR,
                        (errcode(ERRCODE_SYNTAX_ERROR),
                         errmsg("conflicting or redundant options"),
                         parser_errposition(pstate, defel->location)));

best regards.
Ranier Vilela

________________________________________
De: Tom Lane <tgl@sss.pgh.pa.us>
Enviado: sexta-feira, 22 de novembro de 2019 22:17
Para: Ranier Vilela
Cc: pgsql-hackers@postgresql.org
Assunto: Re: [PATCH] Tiny optmization.

Ranier Vilela <ranier_gyn@hotmail.com> writes:
> Redudant because he it's been dereferenced here:
> line 3410:
>         cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);

Not necessarily ... the rel!=NULL code path there doesn't touch pstate,
and that seems to be what contrib/file_fdw is relying on.

Arguably, the rel==NULL code path in BeginCopy should be prepared to
support pstate being null, too.  But what you proposed here is certainly
not OK.

                        regards, tom lane

Re: [PATCH] Tiny optmization.

From
Tomas Vondra
Date:
On Fri, Nov 22, 2019 at 10:24:05PM +0000, Ranier Vilela wrote:
>Hi,
>pstate is touched here:
>a) BeginCopy line 1489:
>    ProcessCopyOptions(pstate, cstate, is_from, options);
>b) ProcessCopyOptions line 1137:
>
>            if (format_specified)
>                ereport(ERROR,
>                        (errcode(ERRCODE_SYNTAX_ERROR),
>                         errmsg("conflicting or redundant options"),
>                         parser_errposition(pstate, defel->location)));
>

And? The fact that you pass a (possibly NULL) pointer somewhere does not
make that a dereference. And parser_errposition() does this:

    if (pstate == NULL || pstate->p_sourcetext == NULL)
        return 0;

So I fail to see why this would be an issue?



>best regards.
>Ranier Vilela
>
>________________________________________
>De: Tom Lane <tgl@sss.pgh.pa.us>
>Enviado: sexta-feira, 22 de novembro de 2019 22:17
>Para: Ranier Vilela
>Cc: pgsql-hackers@postgresql.org
>Assunto: Re: [PATCH] Tiny optmization.
>
>Ranier Vilela <ranier_gyn@hotmail.com> writes:
>> Redudant because he it's been dereferenced here:
>> line 3410:
>>         cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);
>
>Not necessarily ... the rel!=NULL code path there doesn't touch pstate,
>and that seems to be what contrib/file_fdw is relying on.
>
>Arguably, the rel==NULL code path in BeginCopy should be prepared to
>support pstate being null, too.  But what you proposed here is certainly
>not OK.
>
>                        regards, tom lane
>
>

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services