On Mon, 2011-12-19 at 13:23 +0100, Jan Zelený wrote:
> > My recent fix "DP: Remove processed callbacks"
uncovered another bug
> > which needs immediate attention - any NSS operation currently aborts in
> > git HEAD. That's my fault, I only tested my previous fix with cached user
> > data inside the PAM responder where the new bug does not hit. Sorry for
> > that.
> >
> > The problem is memory hierarchy. When user data update is requested
> > from check_cache(), the sss_dp_callback structures are created on
> > sss_domain_context. However, when the callback is called, it goes all the
> > way into nss_cmd_getpw_send_reply() where the command context is freed,
> > freeing also sss_domain_context and the sss_dp_callback data. That's a
> > problem because we are still looping over the callback data.
> >
> > Attached is a patch that changes the memory context for the
> > sss_dp_callback to the client context so it's kept even when the command
> > finishes. The callbacks are freed in the sss_dp_get_account_int_done()
> > function after the callback finishes so we're not keeping them for long.
> >
> > The other solution I was thinking about was to not call the callbacks
> > directly, but only schedule them with tevent_schedule_immediate() so
> > that we can safely loop over all the callbacks, free the sdp_req and
> > then let the clients do whatever they need to.
>
> Ack, pushed to master
>
> Jan
Just a note: I'm not sure the suggested solution with
tevent_schedule_immediate() would solve anything on its own. I didn't go
through the code thoroughly, but I think there might be still a problem when a
callback frees the dctx. Other callbacks would be freed anyway in that case
wouldn't they?
Just to sum up a conversation on IRC: we agreed that the patch as
currently written is the best approach we have available. Changing the
callbacks to use tevent_immediate doesn't appear to gain us much, as we
still have to ensure that they are allocated beneath cli_ctx so that
they can be cancelled on client disconnection.
So (belated) ack to the current patch.