[PATCH] libwbclient clear pointer on bad read
Volker.Lendecke at SerNet.DE
Wed Jan 21 03:05:01 MST 2015
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()
> // do stuff
> main program:
> // ensure all threads have now finished
> // dlclose() here
> or, with the _new and _done functions as well:
> at thread creation:
> at thread exit:
> if (wbc_thread_done() == 0)
> 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!
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
But on the other hand -- working code rulez :-)
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
More information about the samba-technical