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