On Fri, Feb 26, 2016 at 11:08:45AM +0100, Pavel Březina wrote: > On 02/24/2016 03:19 PM, Jakub Hrozek wrote: > >Hi, > > > >the attached patch fixes: > > https://fedorahosted.org/sssd/ticket/2959 > > > >It was confirmed by the original reporter. The bug was there since 2009, > >by the way, I'm really suprised we only caught it now.. > > Good job finding this. I'm inclined to ack this patch, I have one question > though: > > > if (is_user && diff[0]) { > > /* file memberuid removal operations */ > > name = ldb_msg_find_attr_as_string(delop->entry, DB_NAME, NULL); > > if (!name) { > > return LDB_ERR_OPERATIONS_ERROR; > > } > > > > for (i = 0; diff[i]; i++) { > > ret = mbof_append_muop(del_ctx, &del_ctx->muops, > > &del_ctx->num_muops, > > LDB_FLAG_MOD_DELETE, > > diff[i], name, > > DB_MEMBERUID); > > This invokes assignment: del_ctx->muops[j] = diff[i]. To be thorough we > should talloc_steal(del_ctx->muops, diff[i]) here to ensure proper talloc > hierarchy. But it is not necessary since delctx exists only for one > operation... what do you think?
Yes, I think it makes sense.
>From a1d38a29ac76ed79eba1803d6e294bf4281bf114 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Fri, 19 Feb 2016 15:50:12 +0100 Subject: [PATCH] memberof: Don't allocate on a NULL context https://fedorahosted.org/sssd/ticket/2959 In case no previous delete operation occured, the del_ctx->muops pointer we allocate the diff structure was would be NULL, effectivelly leaking the diff array during the memberof processing. Allocating on del_ctx is safer as that pointer is always allocated and prevents the leak. --- src/ldb_modules/memberof.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index 83d93196c34854d75fcd8ac91ad056f64b26b659..54e4b3ee2c74b746e8871cb3bb211bfcb25752e0 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -2145,7 +2145,7 @@ static int mbof_del_mod_entry(struct mbof_del_operation *delop) if (!el || !el->num_values) { return LDB_ERR_OPERATIONS_ERROR; } - diff = talloc_array(del_ctx->muops, struct ldb_dn *, + diff = talloc_array(del_ctx, struct ldb_dn *, el->num_values + 1); if (!diff) { return LDB_ERR_OPERATIONS_ERROR; @@ -2241,6 +2241,7 @@ static int mbof_del_mod_entry(struct mbof_del_operation *delop) if (ret != LDB_SUCCESS) { return ret; } + talloc_steal(del_ctx->muops, diff[i]); } } -- 2.4.3
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
