Re: Error reporting should be improved

[Peter Rosin]

Den 2009-12-30 17:37 skrev 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.

Aha, you have misunderstood the patch, that's why we don't understand
each other. The patch does indeed allocate a buffer, that's the whole
point of sending the error message to lt_dladderror. The error message
is cached indefinitely inside lt_dladderror. In order to not grow that
"cache", I implemented a comparison so that registering the same user
error multiple times results in returning the index of the old error
message instead of inventing a new error number each time.

Note that user_error_strings is an array of strings, not one big long
string.

> > > 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.

This origin idea is completely orthogonal to what the patch is doing.

> > 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 patch does exactly the first part, but not the second. There is simply
no way to know when the user is done with the string, so it has to be
kept indefinitely. We could document the lifetime rules, but until then
there is no way to know when the message can be freed.

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

Yes, that is true. But that is not a new problem, just magnified by
increased usage of lt_dladderror.

Cheers,
Peter

--
They are in the crowd with the answer before the question.
> Why do you dislike Jeopardy?