Re: Error reporting should be improved

[Bob Friesenhahn]

On Wed, 30 Dec 2009, Peter Rosin wrote:
> > There used to be some useless effort to make it thread safe. Apparently 
> > that is gone now.  Regardless, there is also a potential re-entrancy issue, 
> > which could be encountered if a legacy OS interface is supported 
> > (re-implemented) via dlopen() or if some other implementation logic (e.g. 
> > lazy symbol resolution) uses dlsym().
>
> I can't say that I understand what you are describing, but dlerror need
> not be reentrant[1], and we do various manipulations of internal state
> during lt_dlopen etc, so I don't see how any argument about reentrancy
> can hold. What am I missing this time?

What I am saying is that an OS may choose to re-implement its legacy 
loader interfaces via dlopen() and since we still provide a loader 
driver for the legacy interface, use of this other loader may scramble 
the error reports from the dlopen() loader.

> This patch does of course not address the original issue, it only
> addresses the issue with dlerror returning a string to the ltdl user
> that might have a limited life span.

The patch does not seem to address that since it does not allocate a 
buffer to store the error string.  What it does address is how to 
collect multiple error messages.

> > Unfortunately, lt_dladderror() is publically exposed so it is difficult to 
> > change its arguments even though it is not documented.
>
> This patch does not change lt_dladderror in a way that I classify as
> significant. Do you disagree?

I don't disagree, but I had suggested that it is quite useful to know 
the origin of the error report.  The origin could have been handled 
via another parameter.

> I have not found anything in your message that "kills" the patch, but
> yet I get the feeling that you do not like it, and I would like to know
> what you think is wrong with it...

It seems to me that these error strings need to be in allocated buffer 
storage so that they don't point to potentially volatile storage. 
Also, there needs to be a way to clear out the existing errors when a 
lt_dlopen*() or lt_dlsym() starts so that the errors only correspond 
to the last requested operation.

The current implementation appears to be a memory leak since nothing 
is freeing the error string pointer (user_error_strings).

Bob
--
Bob Friesenhahn
address@hidden, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/