On 06/16/2016 03:32 PM, Lukas Slebodnik wrote:
> On (16/06/16 10:32), Pavel Březina wrote:
> > On 06/15/2016 09:57 PM, Lukas Slebodnik wrote:
> > > On (15/06/16 21:54), Lukas Slebodnik wrote:
> > > > On (15/06/16 21:08), Lukas Slebodnik wrote:
> > > > > On (15/06/16 19:08), Lukas Slebodnik wrote:
> > > > > > On (15/06/16 14:08), Pavel Březina wrote:
> > > > > > > On 06/15/2016 08:44 AM, Lukas Slebodnik wrote:
> > > > > > > > On (14/06/16 15:30), Pavel Březina wrote:
> > > > > > > > > On 05/16/2016 02:00 PM, Pavel Březina
wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > the patches are finally ready to be
tested and reviewed. It is too huge
> > > > > > > > > > to be sent to the list so please
checkout my fedorapeople or github repo:
> > > > > > > > > >
> > > > > > > > > >
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=backend
> > > > > > > > > >
https://github.com/pbrezina/sssd/tree/backend
> > > > > > > > > >
> > > > > > > > > > Subdomain handlers are not yet
converted so subdomain support is
> > > > > > > > > > disabled, otherwise everything should
work although I'm sure you'll find
> > > > > > > > > > some bugs.
> > > > > > > > > >
> > > > > > > > > > I managed to do some simple tests
(initgroups, authentication) with ldap
> > > > > > > > > > provider so far and will continue
testing and fixing so if you find a
> > > > > > > > > > bug make sure you run with the latest
version before reporting it please
> > > > > > > > > > :-)
> > > > > > > > > >
> > > > > > > > > > Since the changes touch almost all
areas of SSSD I encourage everyone to
> > > > > > > > > > run and try. Some handlers were
converted quite easily, some took more
> > > > > > > > > > handy work. Areas that are most likely
to contain some bugs are these
> > > > > > > > > > (please give it extra attention):
> > > > > > > > > >
> > > > > > > > > > - proxy provider
> > > > > > > > > > - if group membership changes during
initgroups, nss memory cache should
> > > > > > > > > > be clear through dbus call
> > > > > > > > > > - selinux support
> > > > > > > > > > - hbac support
> > > > > > > > > > - change password
> > > > > > > > > > - password migration (ipa)
> > > > > > > > > >
> > > > > > > > > > Don't be alarmed with the number of
new lines -- there is not that many
> > > > > > > > > > changes. I copied all touched files and
suffixed them with _new so sssd
> > > > > > > > > > can be compiled and kept working until
the latest patches. I also wanted
> > > > > > > > > > to keep the original code intact for
comparison (it was easier for
> > > > > > > > > > development and it may be handy for bug
chasing), you can simply use
> > > > > > > > > > some diff tool to see the changes. We
can squash it in the end.
> > > > > > > > > >
> > > > > > > > > > When I will be confident that the
patches are stable I will do some
> > > > > > > > > > clean up and remove content that is no
longer needed.
> > > > > > > > >
> > > > > > > > > Just a quick update: all found bugs were
fixed, CI pass, all downstream tests
> > > > > > > > > which were tried (I think all but IPA)
pass.
> > > > > > > >
> > > > > > > > 30 errno_t dp_host_handler(struct
sbus_request *sbus_req,
> > > > > > > > 31 void *dp_cli,
> > > > > > > > 32 uint32_t
dp_flags,
> > > > > > > > 33 const char
*name,
> > > > > > > > 34 const char
*alias)
> > > > > > > > 35 {
> > > > > > > > 36 struct dp_hostid_data *data;
> > > > > > > > 37 const char *key;
> > > > > > > > 38
> > > > > > > > 39 if (name == NULL) {
> > > > > > > > 40 return EINVAL;
> > > > > > > > 41 }
> > > > > > > > 42
> > > > > > > > 43 data = talloc_zero(sbus_req, struct
dp_hostid_data);
> > > > > > > > 44 if (data == NULL) {
> > > > > > > > 45 return ENOMEM;
> > > > > > > > 46 }
> > > > > > > > 47
> > > > > > > > 48 data->name = name;
> > > > > > > > 49 data->alias = alias[0] ==
'\0' ? NULL : alias;
> > > > > > > > 50
> > > > > > > > 51 key =
talloc_asprintf("%s:%s", name, (alias == '\0' ? "(null)" :
alias));
> > > > > > > > ^^^
> > > > > > > > The 1st argument
should be a talloc context
> > > > > > > > otherwise it will
CRASH.
> > > > > > > >
> > > > > > > > 52 if (key == NULL) {
> > > > > > > > 53 talloc_free(data);
> > > > > > > > 54 return ENOMEM;
> > > > > > > > 55 }
> > > > > > >
> > > > > > > Thanks fixed.
> > > > > > >
> > > > > > > >
> > > > > > > > I can see a gcc warning on my fedora 24
> > > > > > > > src/providers/ad/ad_subdomains_new.c: In function
‘ad_subdomains_refresh_root_done’:
> > > > > > > > src/providers/ad/ad_subdomains_new.c:575:23:
warning: ‘root_attrs’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > > > > > state->root_attrs = root_attrs;
> > > > > > > > ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> > > > > > > > src/providers/ad/ad_subdomains_new.c:1161:25:
note: ‘root_attrs’ was declared here
> > > > > > > > struct sysdb_attrs *root_attrs;
> > > > > > > > ^~~~~~~~~~
> > > > > > > > src/providers/ad/ad_subdomains_new.c:1194:14:
warning: ‘root_id_ctx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > > > > > subreq = ad_get_slave_domain_send(state,
state->ev, state->sd_ctx,
> > > > > > > >
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > >
root_attrs, root_id_ctx->ldap_ctx);
> > > > > > > >
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > > I seen this in Coverity also reported with root_attrs
maybe uninitialized. It
> > > > > > > seems to me as false positive but maybe I miss
something?
> > > > > > >
> > > > > > It's verly likely some gcc-6 otimisation.
> > > > > > I will try to take a look.
> > > > > >
> > > > > I think that gcc want to tell us this
> > > > >
> > > > > 1156 static void ad_subdomains_refresh_root_done(struct
tevent_req *subreq)
> > > > > 1157 {
> > > > > 1158 struct ad_subdomains_refresh_state *state;
> > > > > 1159 struct tevent_req *req;
> > > > > 1160 struct ad_id_ctx *root_id_ctx;
> > > > > 1161 struct sysdb_attrs *root_attrs;
> > > > > 1162 int dp_error;
> > > > > 1163 errno_t ret;
> > > > > 1164
> > > > > 1165 req = tevent_req_callback_data(subreq, struct
tevent_req);
> > > > > 1166 state = tevent_req_data(req, struct
ad_subdomains_refresh_state);
> > > > > 1167
> > > > > 1168 ret = ad_get_root_domain_recv(state, subreq,
&root_attrs, &root_id_ctx);
> > > > > 1169 talloc_zfree(subreq);
> > > > > 1170 if (ret != EOK) {
> > > > > 1171 DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get
forest root [%d]: %s\n",
> > > > > 1172 ret, sss_strerror(ret));
> > > > > 1173 /* We continue to finish sdap_id_op. */
> > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > root_attrs and root_id_ctx needn't be
initialized after this line.
> > > > > 1174 }
> > > > > 1175
> > > >
> > > > hmm,
> > > > maybe no.
> > > >
> > > > I tried to initialize variables after DEBUG message and it was not
enough.
> > > > I still could see a warnings.
> > > >
> > > > After following change I could see warning only for root_id_ctx.
> > > >
> > > > diff --git a/src/providers/ad/ad_subdomains_new.c
b/src/providers/ad/ad_subdomains_new.c
> > > > index 83539a1..09cc985 100644
> > > > --- a/src/providers/ad/ad_subdomains_new.c
> > > > +++ b/src/providers/ad/ad_subdomains_new.c
> > > > @@ -1175,7 +1175,9 @@ static void
ad_subdomains_refresh_root_done(struct tevent_req *subreq)
> > > >
> > > > ret = ad_get_root_domain_recv(state, subreq, &root_attrs,
&root_id_ctx);
> > > > talloc_zfree(subreq);
> > > > + root_attrs = NULL;
> > > > if (ret != EOK) {
> > > > + root_id_ctx = NULL;
> > > > DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get forest root
[%d]: %s\n",
> > > > ret, sss_strerror(ret));
> > > > /* We continue to finish sdap_id_op. */
> > > >
> > > > I went deeper and compiler does not like macro
TEVENT_REQ_RETURN_ON_ERROR.
> > > > according to compiler TEVENT_REQ_RETURN_ON_ERROR could return 0
> > > >
> > > > Here is "version after expansion" of macro
TEVENT_REQ_RETURN_ON_ERROR
> > > > diff --git a/src/providers/ad/ad_subdomains_new.c
b/src/providers/ad/ad_subdomains_new.c
> > > > index 9a2f035..83539a1 100644
> > > > --- a/src/providers/ad/ad_subdomains_new.c
> > > > +++ b/src/providers/ad/ad_subdomains_new.c
> > > > @@ -969,7 +969,15 @@ static errno_t
ad_get_root_domain_recv(TALLOC_CTX *mem_ctx,
> > > > struct ad_get_root_domain_state *state = NULL;
> > > > state = tevent_req_data(req, struct ad_get_root_domain_state);
> > > >
> > > > - TEVENT_REQ_RETURN_ON_ERROR(req);
> > > > + enum tevent_req_state TRROEstate;
> > > > + uint64_t TRROEerr;
> > > > +
> > > > + if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) {
> > > > + if (TRROEstate == TEVENT_REQ_USER_ERROR) {
> > > > + return TRROEerr;
> > > > + }
> > > > + return ERR_INTERNAL;
> > > > + }
> > > >
> > > > *_attrs = talloc_steal(mem_ctx, state->root_domain_attrs);
> > > > *_id_ctx = state->root_id_ctx;
> > > >
> > > >
> > > > and here is diff on top of previous one which makes complier happy
> > > > diff --git a/src/providers/ad/ad_subdomains_new.c
b/src/providers/ad/ad_subdomains_new.c
> > > > index 83539a1..eab1972 100644
> > > > --- a/src/providers/ad/ad_subdomains_new.c
> > > > +++ b/src/providers/ad/ad_subdomains_new.c
> > > > @@ -970,10 +970,15 @@ static errno_t
ad_get_root_domain_recv(TALLOC_CTX *mem_ctx,
> > > > state = tevent_req_data(req, struct ad_get_root_domain_state);
> > > >
> > > > enum tevent_req_state TRROEstate;
> > > > - uint64_t TRROEerr;
> > > > + uint64_t TRROEuint64;
> > > > + errno_t TRROEerr;
> > > >
> > > > - if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) {
> > > > + if (tevent_req_is_error(req, &TRROEstate, &TRROEuint64))
{
> > > > + TRROEerr = (errno_t) TRROEuint64;
> > > > if (TRROEstate == TEVENT_REQ_USER_ERROR) {
> > > > + if (TRROEerr == 0) {
> > > > + return EINVAL;
> > > > + }
> > > > return TRROEerr;
> >
> > Isn't this change sufficient?
> >
> No because gcc still correctly assume that
> output arguments are not initialized in case of error.
>
> and we do not terminate execution in function ad_subdomains_refresh_root_done
> if ad_get_root_domain_recv fails.
>
> > > > }
> > > > return ERR_INTERNAL;
> > > > @@ -1179,6 +1184,8 @@ static void
ad_subdomains_refresh_root_done(struct tevent_req *subreq)
> > > > DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get forest root
[%d]: %s\n",
> > > > ret, sss_strerror(ret));
> > > > /* We continue to finish sdap_id_op. */
> > > > + root_attrs = NULL;
> > > > + root_id_ctx = NULL;
> > > > }
> > > >
> > > > /* We finish sdap_id_op here since we connect
> > > >
> > >
> > > I forgot to write that another way how to silnce compiler is to
> > > do not declare function ad_get_root_domain_recv as static.
> >
> > So I suppose it tries to inline the function and then screws up?
> >
> yes,
> but compiler is not tottaly wrong.
>
> LS
Thank you. Do you want to send the patch for the tevent macro then?
nitpick:
/sssd/src/providers/data_provider/dp_request_reply.c: In function
‘dp_req_with_reply_step’:
/sssd/src/providers/data_provider/dp_request_reply.c:260: warning:
declaration of ‘request_key’ shadows a global declaration
/usr/include/keyutils.h:104: warning: shadowed declaration is here