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?