On (16/06/16 11:13), Jakub Hrozek wrote: >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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
