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?