Thread: [BUG] Uninitializaed configOut.leafType used.
Hi, file: /backend/access/spgist/spgvalidate.c line: 159 var: configOut.leafType Best regards, Ranier Vilela
Ranier Vilela <ranier_gyn@hotmail.com> writes:
> Hi,
> file: /backend/access/spgist/spgvalidate.c
> line: 159
> var: configOut.leafType
TBH, I think whatever tool you're using to detect these things
is buggy. We don't get compiler warnings about it from any
compiler in common use among pghackers, and what might be
more to the point, we don't get Valgrind complaints.
If you want us to take these complaints seriously, you need
to provid more than zero evidence why we should.
regards, tom lane
Hi,
Ok, so all these commands are for what?
case SPGIST_CONFIG_PROC:
ok = check_amproc_signature(procform->amproc, VOIDOID, true,
2, 2, INTERNALOID, INTERNALOID);
configIn.attType = procform->amproclefttype;
memset(&configOut, 0, sizeof(configOut));
OidFunctionCall2(procform->amproc,
PointerGetDatum(&configIn),
PointerGetDatum(&configOut));
configOutLefttype = procform->amproclefttype;
configOutRighttype = procform->amprocrighttype;
/*
* When leaf and attribute types are the same, compress
* function is not required and we set corresponding bit in
* functionset for later group consistency check.
*/
if (!OidIsValid(configOut.leafType) ||
configOut.leafType == configIn.attType)
{
When case SPGIST_CONFIG_PROC, OidIsValid(configOut.leafType) is tested,
but when case SPGIST_COMPRESS_PROC is fired, OidIsValid(configOut.leafType) is not necessary and configOut.leafType
happy accessed?
Maybe, the Valgrind paths, didn't have a chance, with case SPGIST_COMPRESS_PROC.
I review carefully, before send to list and premature judgments, don't help.
Best regards,
Ranier Vilela
________________________________________
De: Tom Lane <tgl@sss.pgh.pa.us>
Enviado: quarta-feira, 13 de novembro de 2019 17:18
Para: Ranier Vilela
Cc: pgsql-bugs@lists.postgresql.org
Assunto: Re: [BUG] Uninitializaed configOut.leafType used.
Ranier Vilela <ranier_gyn@hotmail.com> writes:
> Hi,
> file: /backend/access/spgist/spgvalidate.c
> line: 159
> var: configOut.leafType
TBH, I think whatever tool you're using to detect these things
is buggy. We don't get compiler warnings about it from any
compiler in common use among pghackers, and what might be
more to the point, we don't get Valgrind complaints.
If you want us to take these complaints seriously, you need
to provid more than zero evidence why we should.
regards, tom lane
Hi,
On 2019-11-13 17:55:40 +0000, Ranier Vilela wrote:
> Ok, so all these commands are for what?
"commands"?
> case SPGIST_CONFIG_PROC:
> ok = check_amproc_signature(procform->amproc, VOIDOID, true,
> 2, 2, INTERNALOID, INTERNALOID);
> configIn.attType = procform->amproclefttype;
> memset(&configOut, 0, sizeof(configOut));
>
> OidFunctionCall2(procform->amproc,
> PointerGetDatum(&configIn),
> PointerGetDatum(&configOut));
>
> configOutLefttype = procform->amproclefttype;
> configOutRighttype = procform->amprocrighttype;
>
> /*
> * When leaf and attribute types are the same, compress
> * function is not required and we set corresponding bit in
> * functionset for later group consistency check.
> */
> if (!OidIsValid(configOut.leafType) ||
> configOut.leafType == configIn.attType)
> {
>
> When case SPGIST_CONFIG_PROC, OidIsValid(configOut.leafType) is tested,
> but when case SPGIST_COMPRESS_PROC is fired, OidIsValid(configOut.leafType) is not necessary and configOut.leafType
> happy accessed?
Even if that were a problem - and I don't see why - that'd still not
make configOut.leafType be uninitialized. The SPGIST_CONFIG_PROC case is
always hit before SPGIST_COMPRESS_PROC, therefore configOut is always
initialized (c.f. memset(0) and the call to amproc to initialize the
contents).
And the OidIsValid() call is about whether a leafType is set or not, the
call to check_amproc_signature() doesn't need that.
- Andres
Hi,
Now we have a technical explanation, which explains the problem.
And not a judgment about knowledge.
Let's hope that always SPGIST_CONFIG_PROC hit first.
Best regards.
Ranier Vilela
________________________________________
De: Andres Freund <andres@anarazel.de>
Enviado: quarta-feira, 13 de novembro de 2019 18:03
Para: Ranier Vilela
Cc: pgsql-bugs@lists.postgresql.org
Assunto: Re: [BUG] Uninitializaed configOut.leafType used.
Hi,
On 2019-11-13 17:55:40 +0000, Ranier Vilela wrote:
> Ok, so all these commands are for what?
"commands"?
> case SPGIST_CONFIG_PROC:
> ok = check_amproc_signature(procform->amproc, VOIDOID, true,
> 2, 2, INTERNALOID,
INTERNALOID);
> configIn.attType = procform->amproclefttype;
> memset(&configOut, 0, sizeof(configOut));
>
> OidFunctionCall2(procform->amproc,
> PointerGetDatum(&configIn),
> PointerGetDatum(&configOut));
>
> configOutLefttype = procform->amproclefttype;
> configOutRighttype = procform->amprocrighttype;
>
> /*
> * When leaf and attribute types are the same, compress
> * function is not required and we set corresponding bit in
> * functionset for later group consistency check.
> */
> if (!OidIsValid(configOut.leafType) ||
> configOut.leafType == configIn.attType)
> {
>
> When case SPGIST_CONFIG_PROC, OidIsValid(configOut.leafType) is tested,
> but when case SPGIST_COMPRESS_PROC is fired, OidIsValid(configOut.leafType) is not necessary and configOut.leafType
> happy accessed?
Even if that were a problem - and I don't see why - that'd still not
make configOut.leafType be uninitialized. The SPGIST_CONFIG_PROC case is
always hit before SPGIST_COMPRESS_PROC, therefore configOut is always
initialized (c.f. memset(0) and the call to amproc to initialize the
contents).
And the OidIsValid() call is about whether a leafType is set or not, the
call to check_amproc_signature() doesn't need that.
- Andres
On 2019-11-13 18:11:35 +0000, Ranier Vilela wrote: > Now we have a technical explanation, which explains the problem. > And not a judgment about knowledge. I have no idea what that means. > Let's hope that always SPGIST_CONFIG_PROC hit first. It will be. The list of support functions is is essentially ordered by amprocnum. And the procedure numbers are: /* SPGiST opclass support function numbers */ #define SPGIST_CONFIG_PROC 1 #define SPGIST_CHOOSE_PROC 2 #define SPGIST_PICKSPLIT_PROC 3 #define SPGIST_INNER_CONSISTENT_PROC 4 #define SPGIST_LEAF_CONSISTENT_PROC 5 #define SPGIST_COMPRESS_PROC 6 #define SPGISTNRequiredProc 5 #define SPGISTNProc 6 It certainly could be more obvious. I'm not a fan of the spgist code :(. Greetings, Andres Freund
On Wed, Nov 13, 2019 at 05:55:40PM +0000, Ranier Vilela wrote:
>Hi,
>Ok, so all these commands are for what?
>
> case SPGIST_CONFIG_PROC:
> ok = check_amproc_signature(procform->amproc, VOIDOID, true,
> 2, 2, INTERNALOID, INTERNALOID);
> configIn.attType = procform->amproclefttype;
> memset(&configOut, 0, sizeof(configOut));
>
> OidFunctionCall2(procform->amproc,
> PointerGetDatum(&configIn),
> PointerGetDatum(&configOut));
>
> configOutLefttype = procform->amproclefttype;
> configOutRighttype = procform->amprocrighttype;
>
> /*
> * When leaf and attribute types are the same, compress
> * function is not required and we set corresponding bit in
> * functionset for later group consistency check.
> */
> if (!OidIsValid(configOut.leafType) ||
> configOut.leafType == configIn.attType)
> {
>
>When case SPGIST_CONFIG_PROC, OidIsValid(configOut.leafType) is tested,
>but when case SPGIST_COMPRESS_PROC is fired, OidIsValid(configOut.leafType) is not necessary and configOut.leafType
>happy accessed?
>
>Maybe, the Valgrind paths, didn't have a chance, with case SPGIST_COMPRESS_PROC.
>
>I review carefully, before send to list and premature judgments, don't help.
>
That's great, but unfortunately it's useless if you don't explain the
reasoning while reporting the issue. Even better if you can provide a
reproducer hitting the issue, of course.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andres Freund <andres@anarazel.de> writes:
> It certainly could be more obvious. I'm not a fan of the spgist code :(.
Yeah. The claimed problem is unreachable, because the only way to get
there is for configOutLefttype/configOutRighttype to have been set to
something valid by a previous execution of the SPGIST_CONFIG_PROC case.
However, if the proclist members don't appear in numerical sequence,
we'd get a bogus failure because the "ok = false" path would be taken.
That "if" is true normally because of the way the catcache works, but
I think it's possible to make it fail if you disable system index usage.
Not a realistic scenario in practice, but less robust than I'd like.
Can't get terribly excited about fixing it though.
regards, tom lane