[PATCH] libwbclient clear pointer on bad read

[Matthew Newton]

Hi,

On Wed, Jan 21, 2015 at 11:05:01AM +0100, Volker Lendecke wrote:
> 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.


I'm starting to think that going for this method might be the best
way to do it, if I can get it done in a reasonable amount of time
anyway. Just need to clarify exactly what you have in mind before
I spend time writing code!

Are you saying that the API would be changed like this, e.g.:

Current:

  struct wbcAuthUserInfo *info = NULL;
  struct wbcAuthErrorInfo *error = NULL;

  authparams.account_name = "testuser";
  ... etc

  err = wbcAuthenticateUserEx(&authparams, &info, &error);

New:

  struct wbcAuthUserInfo *info = NULL;
  struct wbcAuthErrorInfo *error = NULL;
// ctx is local to the thread etc:
  static struct wbcContext *ctx = NULL;

  if (!ctx) {
    // or maybe this should just be a talloc in the calling
    // application?
    ctx = wbcNewContext();
  }

  authparams.account_name = "testuser";
  ... etc

  err = wbcAuthenticateUserEx(ctx, &authparams, &info, &error);



  ... at end of usage:

  wbcFreeContext(ctx);

which would mean updating all the public functions in libwbclient
to have an extra 'ctx' passed to them?

The wbcContext would contain the fd, is_privileged, etc data, so
all held in the calling process/thread.

Or did you have something else in mind?



> > The other possibility I can think of would be the following:
...
> > 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
> magic.
> 
> But on the other hand -- working code rulez :-)

I've got (not quite ready to compile) code using this more basic
method, and it doesn't actually look complicated TBH, no real
hidden magic. In some ways it's simpler than the previous version
as it means the library knows it's threaded because the calling
application tells it so, so can initialise things at thread
creation time rather than magically if the pointers are NULL.

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>