On (16/06/16 10:32), Pavel Březina wrote:
> On 06/15/2016 09:57 PM, 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;
>
> Isn't this change sufficient?
>
No because gcc still correctly assume that
output arguments are not initialized in case of error.
and we do not terminate execution in function ad_subdomains_refresh_root_done
if ad_get_root_domain_recv fails.
>>> }
>>> 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.
>
> So I suppose it tries to inline the function and then screws up?
>
yes,
but compiler is not tottaly wrong.
LS
Thank you. Do you want to send the patch for the tevent macro then?