Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
Pavel Stehule pavel.steh...@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 - 1.98
--- dfmgr.c 1 Apr 2009 23:41:37 -
***
*** 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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
2009/4/2 Tom Lane t...@sss.pgh.pa.us:
Pavel Stehule pavel.steh...@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 - 1.98
--- dfmgr.c 1 Apr 2009 23:41:37 -
***
*** 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 */
!
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
Pavel Stehule pavel.steh...@gmail.com writes: 2009/4/2 Tom Lane t...@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
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
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 t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@gmail.com writes: 2009/4/2 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
2009/4/2 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2009/4/2 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] protect dll lib initialisation against any exception, for 8.5
Greg Stark greg.st...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
*** ./src/backend/utils/fmgr/dfmgr.c.orig 2009-03-31 20:32:21.0 +0200
--- ./src/backend/utils/fmgr/dfmgr.c 2009-03-31 21:59:36.0 +0200
***
*** 281,287
*/
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)
--- 281,313
*/
PG_init = (PG_init_t) pg_dlsym(file_scanner-handle, _PG_init);
if (PG_init)
! {
! MemoryContext oldcontext = CurrentMemoryContext;
!
! PG_TRY();
! {
! /* try to init library */
! (*PG_init) ();
! }
! PG_CATCH();
! {
! ErrorData *edata;
!
! MemoryContextSwitchTo(oldcontext);
! edata = CopyErrorData();
! FlushErrorState();
!
! /* we can't to use PG_RE_THROWN after close the originator file */
! clear_external_function_hash(file_scanner-handle);
! pg_dlclose(file_scanner-handle);
! free((char *) file_scanner);
!
! ereport(ERROR,
! (errmsg(cannot initialise library \%s\, libname),
! errhint(Function _PG_init throws exception \%s\., edata-message)));
! }
! PG_END_TRY();
! }
/* OK to link it into list */
if (file_list == NULL)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
