[PATCH] libwbclient clear pointer on bad read

[Volker Lendecke]

On Fri, Jan 09, 2015 at 02:33:28PM +0000, Matthew Newton wrote:
> Hi,
> 
> On Fri, Jan 09, 2015 at 12:42:44PM +0100, Volker Lendecke wrote:
> > On Thu, Jan 08, 2015 at 03:59:48PM +0000, Matthew Newton wrote:
> > > In nsswitch/wb_common.c, in winbindd_read_reply: if
> > > winbind_read_sock fails for some reason and returns -1 then it's
> > > possible that some data could have actually been read. Therefore
> > > the extra_data.data pointer can be set. The attached simple patch
> > > moves clearing this pointer to above the return calls so that the
> > > later free in the calling code won't ever crash out trying to free
> > > an invalid pointer.
> > 
> > Yes, correct. I'd say that the callers are buggy if they try to free
> > something when request_response did not return successfully,
> 
> Actually, it's libwbclient itself that does the call. Backtrace is:

Yep, that's what I thought.

> wb_common.c:543: winbindd_read_reply
>  (returns -1 on error)
> wb_common.c:648: winbindd_get_response
>  (returns NSS_STATUS_UNAVAIL on err)
> wb_common.c:700: winbindd_priv_request_response
> libwbclient/wbclient.c:50: wbcRequestResponseInt
>  (returns WBC_ERR_WINBIND_NOT_AVAILABLE on err)
> libwbclient/wbclient.c:100: wbcRequestResponsePriv
> libwbclient/wbc_pam.c:345: wbcAuthenticateUserEx
> 
> So wbcAuthenticateUserEx gets "WBC_ERR_WINBIND_NOT_AVAILABLE" when
> the read failed. I guess there's an additional error check that
> should be added to libwbclient/wbc_pam.c (noting that
> BAIL_ON_WBC_ERROR does a "goto done", and winbindd_free_response()
> will try and free extra_data.data if it's not NULL, hence the
> original issue):

Ok, the BAIL_ON_WBC_ERROR insanity must be extincted. Your
patch is right of course.

> > > I guess the library just needs to somehow keep these two variables
> > > local to the thread if compiled with pthreads?
> > 
> > Either that or just allow one outstanding request. I'm not sure which
> > one is easier to do.
> 
> >From my view, only ensuring one outstanding request defeats the
> point of what I'm trying to do. We're hitting issues where our
> wireless authentication is unable to cope with the load, so rather
> than running ntlm_auth for each authentication I'd like to just
> link directly to the library which allows the auths to complete a
> bit faster to get a bit more throughput, as well as just being a
> much neater way to do the auth.
>
> Putting a mutex around the call to wbcAuthenticateUserEx
> definitely makes it work safely, but is then actually worse than
> execing multiple ntlm_auths from many threads in terms of
> throughput. :)

Ok. True.

> > For the per-thread fd I'm not sure how to properly take care of
> > cleaning up if we have many threads. libwbclient is used in
> > pam_winbind, and we can't afford to leak fds there. See for
> 
> OK, that's useful to know. I've searched high and low and so far
> couldn't actually find anything that was actually using
> libwbclient to date!

smbd itself uses it.

> > example the destructor attribute attached to
> > winbind_close_sock(). This would become much more difficult to
> > do.
> > 
> > In libwbclient there are a few more global variables, look for example at
> > "pw_cache_size" & following in libwbclient/wbc_pwd.c. I did not really
> > pursue thread-safety for libwbclient lately anymore, but I'd certainly
> > be happy to look at patches!
> 
> Hmm, yes - I'm not really familiar with Samba code yet - we could
> maybe use thread local storage and define these as e.g.
> 
> __thread int winbindd_fd = -1;
> 
> conditionally dependent on if __thread is available? Looks like
> __thread is available on all recent gcc and clang.

__thread -- I'd feel better with the explicit pthread calls
around pthread_setspecific which are standardized.

> It's quick so I think I'll try that here and see if it fixes at
> least the issue I'm seeing.
> 
> In terms of the fd destructor, the other option it would seem to
> be to use pthread_key_create with a destructor, use
> thread-specific data via that mechanism.
> 
> Do either of these methods look acceptable before I commit to a
> few hours hacking?

A prototype with compiler-specific extension is certainly
highly interesting, but for upstream I'd rather go with
something completely standard.

An alternative approach, which I'd call a long-term goal
though, would be a separate layer below the current
libwbclient which takes a context pointer holding the fd and
all the global variables. This libwbclient layer could be
completely thread-agnostic and not deal with these issues at
all, looking only ever at the context. Each context would
have to be externally serialized, but we would not have to
deal with threading issues inside that libwbclient layer at
all. The current published API would hold one such context
structure (either per thread or globally per process,
properly mutexed).

This would be my preferred approach, but if you need
something quicker, probably the pthread_setspecific approach
might be okay.

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