[PATCH] libwbclient clear pointer on bad read

[Volker Lendecke]

On Tue, Jan 20, 2015 at 10:41:53PM +0000, Matthew Newton wrote:
> One recommended solution it to store all the library state in the
> calling application, and pass it in to the library each time. This
> is tidier, but probably not as good as it requires an API change.
> Having said that, as the library looks like it is mostly only used
> within Samba itself, this might be an option.

This is pretty much what I had in mind with the two layers.
One layer takes a libwbclient context, and the compat-layer
above stores one such context in a mutexed way. Only the
lower layer with a libwbclient context with its new API
would be able to cope with multiple threads in parallel.

> 
> 
> The other possibility I can think of would be the following:
> 
>  - introduce a new global variable, bool is_threaded = false;
> 
>  - add wbc_thread_init() that:
>     sets is_threaded to true
>     calls pthread_key_create()
> 
>  - add wbc_thread_finish() that:
>     calls pthread_key_delete()
>     sets is_threaded to false
> 
>  - update winbindd_fd_info and friends to return pointer to
>    global variable if is_threaded is false, or handle TLS if
>    is_threaded is true.
> 
> (possibly also) add wbc_thread_new() and wbc_thread_done() that
> each application thread should call at creation/exit - these would
> keep a counter of the number of threads in use, to check at
> wbc_thread_finish() time so we know all threads are done.
> 
> This would mean that:
> 
>  - Normally the library would behave exactly as now, not
>    thread-safe, and using global variables. Destructor would call
>    close(winbindd_fd) etc as current. No memory to leak for
>    existing applications.
> 
>  - wbc_thread_init() would call pthread_key_create, saving a call
>    to pthread_once on every attempt to get the fd.
> 
>  - threaded applications are safe to dlclose() the library,
>    provided that all threads have finished and wbc_thread_finish()
>    has been called.
> 
>  - as threads exit, the TLS destructor is called and frees up the
>    TLS data.
> 
>  - the only API change is for threaded applications, which is a
>    new use case so nothing to worry about.
> 
> 
> Use case without _new and _done:
> 
>   main program:
>     // dlopen()
>     wbc_thread_init()
> 
>   threads:
>     // do stuff
> 
>   main program:
>     // ensure all threads have now finished
>     wbc_thread_finish()
>     // dlclose() here
> 
> or, with the _new and _done functions as well:
> 
>   at thread creation:
>     pthread_once(wbc_thread_init)
>     wbc_thread_new()
> 
>   at thread exit:
>     if (wbc_thread_done() == 0)
>       wbc_thread_finish();
> 
> Is that a reasonable way to do it to keep things safe? The more I
> think about it, the less complicated it seems - most of the code
> is about the same as now. But it's possible I'm way off the mark!
> :-)
> 
> 
> Thoughts?

While both would probably work fine, my preference would be
the layered approach. More intrusive change API-wise, but
from my point of view much cleaner. If there's one thing
that I really, really would like to avoid then it's hidden
magic.

But on the other hand -- working code rulez :-)

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de