Re: pg_migrator issue with contrib - Mailing list pgsql-hackers

On Sun, Jun 7, 2009 at 12:36 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> * pageinspect has changed the ABI of get_raw_page() in a way that will
>> likely make it dump core if the function definition is migrated from
>> an old DB.  This needs to be fixed.
>> [ and similarly for some other contrib modules ]
>
> After thinking about this some more, I think that there is a fairly
> simple coding rule we can adopt to prevent post-migration crashes
> of the sort I'm worrying about above.  That is:
>
> * If you change the ABI of a C-language function, change its C name.
>
> This will ensure that if someone tries to migrate the old function
> definition from an old database, they will get a pg_migrator failure,
> or at worst a clean runtime failure when they attempt to use the old
> definition.  They won't get a core dump or some worse form of security
> problem.
>
> As an example, the problem in pageinspect is this diff:
>
> ***************
> *** 6,16 ****
>  --
>  -- get_raw_page()
>  --
> ! CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
>  RETURNS bytea
>  AS 'MODULE_PATHNAME', 'get_raw_page'
>  LANGUAGE C STRICT;
>
>  --
>  -- page_header()
>  --
> --- 6,21 ----
>  --
>  -- get_raw_page()
>  --
> ! CREATE OR REPLACE FUNCTION get_raw_page(text, text, int4)
>  RETURNS bytea
>  AS 'MODULE_PATHNAME', 'get_raw_page'
>  LANGUAGE C STRICT;
>
> + CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
> + RETURNS bytea
> + AS $$ SELECT get_raw_page($1, 'main', $2); $$
> + LANGUAGE SQL STRICT;
> +
>  --
>  -- page_header()
>  --
> ***************
>
> The underlying C-level get_raw_page function is still there, but
> it now expects three arguments not two, and will crash if it's
> passed an int4 where it's expecting a text argument.  But the old
> function definition will migrate without error --- there's no way
> for pg_migrator to realize it's installing a security hazard.
>
> The way we should have done this, which I intend to go change it to,
> is something like
>
> CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
> RETURNS bytea
> AS 'MODULE_PATHNAME', 'get_raw_page'
> LANGUAGE C STRICT;
>
> CREATE OR REPLACE FUNCTION get_raw_page(text, text, int4)
> RETURNS bytea
> AS 'MODULE_PATHNAME', 'get_raw_page_3'
> LANGUAGE C STRICT;
>
> so that the old function's ABI is preserved.  Migration of the old
> contrib module will then lead to the 3-argument function not being
> immediately available, but the 2-argument one still works.  Had we not
> wanted to keep the 2-argument form for some reason, we would have
> provided only get_raw_page_3 in the .so file, and attempts to use the
> old function definition would fail safely.
>
> (We have actually seen similar problems before with people trying
> to dump and reload database containing contrib modules.  pg_migrator
> is not creating a problem that wasn't there before, it's just making
> it worse.)
>
> Comments, better ideas?

maybe, get_raw_page_v2, etc?  I suppose you could run into situation
with multiple versions of the function w/same # of arguments?

merlin