Re: Unsafe use of relation->rd_options without checking its type

From: neha khatri <nehakhatri5(at)gmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Unsafe use of relation->rd_options without checking its type
Date: 2016-11-01 00:10:10
Message-ID: CAFO0U+-oeRHyofrWOdwfmDuZxh5E3uqTG85OrcJMe5-fj1JiKQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>
>
> ^
>
> The reason for the error is that transformOnConflictArbiter applies
> RelationIsUsedAsCatalogTable() to something that it doesn't know to
> be a plain relation --- it's a view in this case. And that macro
> blindly assumes that relation->rd_options is a StdRdOptions struct,
> when in this case it's actually a ViewOptions struct.
>
> Now that I've seen this I wonder which other uses of rd_options are
> potentially broken. RelationIsUsedAsCatalogTable() is hardly the
> only macro that is assuming with little justification that it's
> applied to the right kind of reloptions.
>

Right, there are many macros, which blindly assume the rd_options
to be of specific type. Here is the list of such macros :

RelationGetFillFactor
RelationIsUsedAsCatalogTable
RelationGetParallelWorkers
RelationIsSecurityView
RelationHasCheckOption
RelationHasLocalCheckOption
RelationHasCascadedCheckOption
BrinGetPagesPerRange
GinGetUseFastUpdate
GinGetPendingListCleanupSize

For the macros associated with specific index type, it might be alright to
assume the type of the rd_options because those have very limited usage.
However for the rest of the macros, the code does not limit its usage. This
can
lead to problems as you described above .

> We could band-aid this by having the RelationIsUsedAsCatalogTable()
> macro check relation->relkind, but I'm inclined to think that the
> right fix going forward is to insist that StdRdOptions, ViewOptions,
> and the other in-memory representations of reloptions ought to be
> self-identifying somehow. We could add a field to them similar to
> nodeTag, but one of the things that was envisioned to begin with
> is relation types that have StdRdOptions as the first part of a
> larger struct. I'm not sure how to make that work with a tag.
>
> Thoughts?
>

Yes, it seems appropriate to tag all types of the rd_options with an
identification and maybe check for that identification within each macro.
Currently, there are following types of rd_options as far as I could find:

GiSTOptions
BrinOptions
GinOptions
StdRdOptions
ViewOptions
BloomOptions (from contrib)

I am not clear on how the identification field in the above structures can
be a problem, for the relations have StdRdOptions as the first part of a
larger structure.

Regards,
Neha

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Vladimir Gordiychuk 2016-11-01 00:48:37 Re: Logical decoding and walsender timeouts
Previous Message Thomas Munro 2016-11-01 00:02:39 WIP: [[Parallel] Shared] Hash