[SSSD] [PATCHES] check return value

Sumit Bose sbose at redhat.com
Wed Apr 9 19:16:51 UTC 2014


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

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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list