[PATCH] libwbclient clear pointer on bad read

Matthew Newton mcn4 at leicester.ac.uk
Tue Jan 20 15:41:53 MST 2015


On Tue, Jan 20, 2015 at 04:42:22PM +0100, Volker Lendecke wrote:
> It's the single-threaded application with a thread-aware
> libwbclient that I'm worried about.

Yes, it looks like this is an issue. The scenarios:

  library and app compiled without pthreads; no problems, falls
  back to global vars at compile time.

  app using threads, library without: same as current situation.

  app using threads, library with threads: looks fine, but:
   - destructor needs to call pthread_key_delete() [0]
   - dlclose must only happen after all threads have stopped (or
   - link library with -z nodelete, or dlopen with RTLD_NODELETE)

  app not threaded, library with threads:
   - "thread" never exits as there is no thread, so TLS data is
       never free()d (have tested this)
   - destructor needs to call pthread_key_delete(), which the man
     page says "No destructor functions shall be invoked by
     pthread_key_delete()" - I've tested this, and it is true that
     it doesn't call the destructor

[0] it seems like asking the destructor to call
pthread_key_delete() is also not recommended, from what I have
read, as a call to dlclose() will call this, possibly before all
threads have finished, leading to segfault at thread exit time
when it tries to call its (non-existant any more) destructor.
Hence people use -z nodelete to keep the code in memory.


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.


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?

Thanks!

Matthew


-- 
Matthew Newton, Ph.D. <mcn4 at le.ac.uk>

Systems Specialist, Infrastructure Services,
I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom

For IT help contact helpdesk extn. 2253, <ithelp at le.ac.uk>


More information about the samba-technical mailing list