[SSSD] [PATCHES] check return value

Pavel Reichl preichl at redhat.com
Wed Apr 9 15:22:16 UTC 2014


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 at 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:

> 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?

I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.







More information about the sssd-devel mailing list