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
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel@lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > 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.
> >
> >
> >
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel@lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Sumit, thank you for explaining it to me - lesson learned here.
Lukas, would attached patch work better for you?
>From 6a33f72cae764a9f8c95eea813541e45b454c03b Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 4 Apr 2014 14:09:24 +0200
Subject: [PATCH 1/2] PAC: check return value of hash_entries
Fix warning reported by clang - 'dead assignment'.
---
src/responder/pac/pacsrv_cmd.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index 229c30783f265b972dadc9c978edf743731edb9f..00cdf608bef2be1543a15cee4ae02c78c3bc9f8e 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -268,14 +268,20 @@ static void pac_lookup_sids_done(struct tevent_req *req)
if (ret != EOK) {
talloc_free(pr_ctx);
- pac_cmd_done(cctx, ret);
- return;
+ goto error;
}
key.type = HASH_KEY_STRING;
value.type = HASH_VALUE_ULONG;
ret = hash_entries(pr_ctx->sid_table, &count, &entries);
+ if (ret != HASH_SUCCESS) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "hash_entries failed.\n");
+ talloc_free(pr_ctx);
+ ret = ENOMEM;
+ goto error;
+ }
+
for (c = 0; c < count; c++) {
if (entries[c].value.ul == 0) {
ret =responder_get_domain_by_id(cctx->rctx,
@@ -304,8 +310,8 @@ static void pac_lookup_sids_done(struct tevent_req *req)
"for SID [%s].\n",
entries[c].key.str);
talloc_free(msg);
- pac_cmd_done(cctx, EINVAL);
- return;
+ ret = EINVAL;
+ goto error;
}
id = ldb_msg_find_attr_as_uint64(msg->msgs[0],
@@ -335,6 +341,10 @@ static void pac_lookup_sids_done(struct tevent_req *req)
}
pac_add_user_next(pr_ctx);
+ return;
+
+error:
+ pac_cmd_done(cctx, ret);
}
static void pac_add_user_next(struct pac_req_ctx *pr_ctx)
--
1.8.4.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
