Thread: protect dll lib initialisation against any exception, for 8.5
Hello attached patch allows raising exception from _PG_init function as was discussed before. regards Pavel Stehule
Attachment
Pavel Stehule <pavel.stehule@gmail.com> writes:
> attached patch allows raising exception from _PG_init function as was
> discussed before.
I fooled around with this and came up with the attached improved
version, which allows reporting the full error status. However,
after thinking some more I feel that this is probably a cure worse
than the disease. If we simply leave the code as it stands, an
elog(ERROR) in an init function doesn't corrupt dfmgr.c's internal list,
which is what I had been fearing when I complained about the issue.
The worst that happens is that we leave the library loaded and leak
a little bit of memory. Unloading the library, as the patch does,
could easily make things worse not better. Consider the not-unlikely
case that the library installs itself in a few callback hooks and
then fails. If we unload the library, those hooks represent
*guaranteed* core dumps on next use. If we don't unload, the hook
functions might or might not work too well --- presumably not everything
they need has been initialized --- but it's hard to imagine an outcome
that's worse than a guaranteed core dump.
So I'm thinking this is really unnecessary and we should leave well
enough alone.
regards, tom lane
Index: dfmgr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/fmgr/dfmgr.c,v
retrieving revision 1.98
diff -c -r1.98 dfmgr.c
*** dfmgr.c 1 Jan 2009 17:23:51 -0000 1.98
--- dfmgr.c 1 Apr 2009 23:41:37 -0000
***************
*** 178,184 ****
static void *
internal_load_library(const char *libname)
{
! DynamicFileList *file_scanner;
PGModuleMagicFunction magic_func;
char *load_error;
struct stat stat_buf;
--- 178,184 ----
static void *
internal_load_library(const char *libname)
{
! DynamicFileList * volatile file_scanner;
PGModuleMagicFunction magic_func;
char *load_error;
struct stat stat_buf;
***************
*** 277,287 ****
}
/*
! * If the library has a _PG_init() function, call it.
*/
PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
if (PG_init)
! (*PG_init) ();
/* OK to link it into list */
if (file_list == NULL)
--- 277,329 ----
}
/*
! * If the library has a _PG_init() function, call it. Guard against
! * the function possibly throwing elog(ERROR).
*/
PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
if (PG_init)
! {
! MemoryContext oldcontext = CurrentMemoryContext;
!
! PG_TRY();
! {
! (*PG_init) ();
! }
! PG_CATCH();
! {
! ErrorData *edata;
!
! /* fetch the error status so we can change it */
! MemoryContextSwitchTo(oldcontext);
! edata = CopyErrorData();
! FlushErrorState();
!
! /*
! * The const pointers in the error status very likely point
! * at constant strings in the library, which we are about to
! * unload. Copy them so we don't dump core while reporting
! * the error. This might leak a bit of memory but it
! * beats the alternatives.
! */
! if (edata->filename)
! edata->filename = pstrdup(edata->filename);
! if (edata->funcname)
! edata->funcname = pstrdup(edata->funcname);
! if (edata->domain)
! edata->domain = pstrdup(edata->domain);
!
! /* library might have already called some of its functions */
! clear_external_function_hash(file_scanner->handle);
!
! /* try to unlink library */
! pg_dlclose(file_scanner->handle);
! free((char *) file_scanner);
!
! /* complain */
! ReThrowError(edata);
! }
! PG_END_TRY();
! }
/* OK to link it into list */
if (file_list == NULL)
2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> attached patch allows raising exception from _PG_init function as was
>> discussed before.
>
> I fooled around with this and came up with the attached improved
> version, which allows reporting the full error status. However,
> after thinking some more I feel that this is probably a cure worse
> than the disease. If we simply leave the code as it stands, an
> elog(ERROR) in an init function doesn't corrupt dfmgr.c's internal list,
> which is what I had been fearing when I complained about the issue.
> The worst that happens is that we leave the library loaded and leak
> a little bit of memory. Unloading the library, as the patch does,
> could easily make things worse not better. Consider the not-unlikely
> case that the library installs itself in a few callback hooks and
> then fails. If we unload the library, those hooks represent
> *guaranteed* core dumps on next use. If we don't unload, the hook
> functions might or might not work too well --- presumably not everything
> they need has been initialized --- but it's hard to imagine an outcome
> that's worse than a guaranteed core dump.
>
> So I'm thinking this is really unnecessary and we should leave well
> enough alone.
>
I see it. I thing , an safety of this exception should be solved only
by programmer. It's important to release all hooks, and then raise an
exception. It is in developer responsibility.
regards
Pavel Stehule
> regards, tom lane
>
>
> Index: dfmgr.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/utils/fmgr/dfmgr.c,v
> retrieving revision 1.98
> diff -c -r1.98 dfmgr.c
> *** dfmgr.c 1 Jan 2009 17:23:51 -0000 1.98
> --- dfmgr.c 1 Apr 2009 23:41:37 -0000
> ***************
> *** 178,184 ****
> static void *
> internal_load_library(const char *libname)
> {
> ! DynamicFileList *file_scanner;
> PGModuleMagicFunction magic_func;
> char *load_error;
> struct stat stat_buf;
> --- 178,184 ----
> static void *
> internal_load_library(const char *libname)
> {
> ! DynamicFileList * volatile file_scanner;
> PGModuleMagicFunction magic_func;
> char *load_error;
> struct stat stat_buf;
> ***************
> *** 277,287 ****
> }
>
> /*
> ! * If the library has a _PG_init() function, call it.
> */
> PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
> if (PG_init)
> ! (*PG_init) ();
>
> /* OK to link it into list */
> if (file_list == NULL)
> --- 277,329 ----
> }
>
> /*
> ! * If the library has a _PG_init() function, call it. Guard against
> ! * the function possibly throwing elog(ERROR).
> */
> PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
> if (PG_init)
> ! {
> ! MemoryContext oldcontext = CurrentMemoryContext;
> !
> ! PG_TRY();
> ! {
> ! (*PG_init) ();
> ! }
> ! PG_CATCH();
> ! {
> ! ErrorData *edata;
> !
> ! /* fetch the error status so we can change it */
> ! MemoryContextSwitchTo(oldcontext);
> ! edata = CopyErrorData();
> ! FlushErrorState();
> !
> ! /*
> ! * The const pointers in the error status very likely point
> ! * at constant strings in the library, which we are about to
> ! * unload. Copy them so we don't dump core while reporting
> ! * the error. This might leak a bit of memory but it
> ! * beats the alternatives.
> ! */
> ! if (edata->filename)
> ! edata->filename = pstrdup(edata->filename);
> ! if (edata->funcname)
> ! edata->funcname = pstrdup(edata->funcname);
> ! if (edata->domain)
> ! edata->domain = pstrdup(edata->domain);
> !
> ! /* library might have already called some of its functions */
> ! clear_external_function_hash(file_scanner->handle);
> !
> ! /* try to unlink library */
> ! pg_dlclose(file_scanner->handle);
> ! free((char *) file_scanner);
> !
> ! /* complain */
> ! ReThrowError(edata);
> ! }
> ! PG_END_TRY();
> ! }
>
> /* OK to link it into list */
> if (file_list == NULL)
>
>
Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>:
>> So I'm thinking this is really unnecessary and we should leave well
>> enough alone.
> I see it. I thing , an safety of this exception should be solved only
> by programmer. It's important to release all hooks, and then raise an
> exception. It is in developer responsibility.
Well, if the init function is sufficiently carefully coded to back out
just the changes it's managed to apply, then good for it. But we still
aren't losing much by leaving dfmgr as-is.
regards, tom lane
Hmm. One case where this logic might not be true would be if the dll relies on c++ style static initializers and destructors. In that case it may very well leave hooks in place in case of an error and only clean them up when you call dlclose(). -- Greg On 1 Apr 2009, at 22:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> 2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>: >>> So I'm thinking this is really unnecessary and we should leave well >>> enough alone. > >> I see it. I thing , an safety of this exception should be solved only >> by programmer. It's important to release all hooks, and then raise an >> exception. It is in developer responsibility. > > Well, if the init function is sufficiently carefully coded to back out > just the changes it's managed to apply, then good for it. But we > still > aren't losing much by leaving dfmgr as-is. > > regards, tom lane > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> 2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>: >>> So I'm thinking this is really unnecessary and we should leave well >>> enough alone. > >> I see it. I thing , an safety of this exception should be solved only >> by programmer. It's important to release all hooks, and then raise an >> exception. It is in developer responsibility. > > Well, if the init function is sufficiently carefully coded to back out > just the changes it's managed to apply, then good for it. But we still > aren't losing much by leaving dfmgr as-is. > Maybe an safe minimum is cleaning symbols table without closing library. Then the code from lib will be accessible, but functionality will be disabled (for Postgres)? regards Pavel Stehule > regards, tom lane >
Pavel Stehule <pavel.stehule@gmail.com> writes:
> Maybe an safe minimum is cleaning symbols table without closing
> library. Then the code from lib will be accessible, but functionality
> will be disabled (for Postgres)?
If the library doesn't get added to the list in dfmgr.c, we'll never
look for symbols within it anyway. So I don't think there's any
particular cleaning to be done --- even assuming that the platform
supports removing symbols without dlclose'ing the library, which
seems rather unlikely.
regards, tom lane
Greg Stark <greg.stark@enterprisedb.com> writes:
> Hmm. One case where this logic might not be true would be if the dll
> relies on c++ style static initializers and destructors. In that case
> it may very well leave hooks in place in case of an error and only
> clean them up when you call dlclose().
Interesting point, but considering that we don't support or encourage
use of C++ anyway, it shouldn't carry much weight in our estimate
of how an init function is likely to behave.
Also, wouldn't C++ initializers most likely get called by the dynamic
loader itself, not during the PG_init function?
regards, tom lane