[SSSD] [PATCHES] check return value

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 16 06:37:09 UTC 2014


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



More information about the sssd-devel mailing list