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? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
