[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

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

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

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

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

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