On (15/04/14 16:41), Pavel Reichl wrote:
On Thu, 2014-04-10 at 10:37 +0200, Pavel Reichl wrote:
> On Wed, 2014-04-09 at 21:16 +0200, Sumit Bose wrote:
> > On Wed, Apr 09, 2014 at 05:22:16PM +0200, Pavel Reichl wrote:
> > > On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
> > > > On (08/04/14 13:00), Sumit Bose wrote:
> > > > >On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
> > > > >> On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl
wrote:
> > > > >> > Hello,
> > > > >> >
> > > > >> > I noticed these two warnings in clang.
> > > > >> >
> > > > >> > It would be great if the 2nd patch could be checked by
Sumit to make
> > > > >> > sure that the return value wasn't ignored on
purpose.
> > > > >>
> > > > >> yes, I would prefer to ignore errors here. There might be
various cases
> > > > >> were we are not able to resolve a single SID but still can
proceed with
> > > > >> the others.
> > > > >>
> > > > >> The first patch looks good, I'll run some tests and will
give my results
> > > > >> later.
> > > > >
> > > > >ACK to the first patch.
> > > > >
> > > > >bye,
> > > > >Sumit
> > > > >
> > > > Not all return point from function pac_lookup_sids_done were
converted
> > > > using goto.
> > > >
> > > > --
> > > > 305- } else if (msg->count > 1) {
> > > > 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one
result returned " \
> > > > 307- "for SID
[%s].\n",
> > > > 308- entries[c].key.str);
> > > > 309- talloc_free(msg);
> > > > 310: pac_cmd_done(cctx, EINVAL);
> > > > 311- return;
> > > > ^^^^^^^
> > > > here
> > > > 312- }
> > > > 313-
> > > > 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0],
> > > > 315- SYSDB_UIDNUM, 0);
> > > > --
> > > >
> > > > LS
> > >
> > > Thank you Lukas for your input. You complicated the situation one more
> > > time. :-)
> > >
> > > I had more careful look on the code and I found several places that
> > > seems troublesome to me:
> >
> > no, if you add a bit of more code:
> >
> > static errno_t pac_resolve_sids_next(struct pac_req_ctx *pr_ctx)
> > [snip]
> > req = pac_lookup_sids_send(pr_ctx, pr_ctx->cctx->ev, pr_ctx,
> > pr_ctx->pac_ctx, pr_ctx->sid_table);
> > if (req == NULL) {
> > DEBUG(SSSDBG_OP_FAILURE, "pac_lookup_sids_send failed.\n");
> > return ENOMEM;
> > }
> >
> > tevent_req_set_callback(req, pac_lookup_sids_done, pr_ctx);
> >
> > you can see that req is allocated on pr_ctx, i.e. req is a child of
> > pr_ctx. Freeing req does not free pr_ctx (but freeing pr_ctx would free
> > req as well). The hierarchy in the struct is a different one than in the
> > talloc tree.
> >
> > >
> > > > static void pac_lookup_sids_done(struct tevent_req *req)
> > > > {
> > > > struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct
pac_req_ctx);
> > > > struct cli_ctx *cctx = pr_ctx->cctx;
> > > >
> > > > [snip]
> > > >
> > > > ret = pac_lookup_sids_recv(req);
> > > > talloc_zfree(req);
> > > >
> > > > [snip]
> > > > ret = hash_entries(pr_ctx->sid_table, &count,
&entries);
> > >
> > > Is this use of pr_ctx->sid use-after-free bug as it was allocated in req
which was already freed?
> > >
> > >
> > > > talloc_free(pr_ctx);
> > > > pac_cmd_done(cctx, ret);
> > >
> > > cctx points to pr_ctx->cctx, but pr_ctx is freed before call of
pac_cmd_done(cctx, ret) - use-after-free?
> >
> > same here:
> >
> > static errno_t pac_add_pac_user(struct cli_ctx *cctx)
> > [snip]
> > pr_ctx = talloc_zero(cctx, struct pac_req_ctx);
> >
> > pt_ctx is a child of cctx talloc-wise.
> >
> > HTH
> >
> > bye,
> > Sumit
> > >
> > > I don't feel confident enough to make any changes here without approval
of somebody more acquainted with this code.
> > >
> > >
> > >
> > >
>
> Hello Sumit, thank you for explaining it to me - lesson learned here.
>
> Lukas, would attached patch work better for you?
>
Hello,
Lukas, do you like this version of the patch or shall we go with
previous version of patch, which is already ACKed by Sumit?
Your patches do not fix any real problem.
RFEs for 1.12 (or 1.11) have higher priority.
e.g.
https://fedorahosted.org/sssd/ticket/1853
I would need to test patches with static analysers; I can review patches on
Thursday or later.
LS