On Wed, Jun 15, 2016 at 09:57:45PM +0200, 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;
> > }
> > 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.
>
> BTW test_dp_request fails with latest version.
> There are some leak report issues.
Does it mean you agree with Pavel that the Coverity issues are false
positives?
This is not Coverity issue. This is gcc 6 warning.
And as you can see it could happen.
It depends on which value you will use in tevent_req_error.
We do not use higher values then UINT32_MAX. But if someone
would use then it could case a problem.
e.g. tevent_req_error(req, 0x1234567800000000)
LS