[SSSD] [PATCHES] check return value

Pavel Reichl preichl at redhat.com
Tue Apr 15 14:41:12 UTC 2014


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





More information about the sssd-devel mailing list