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.
On (16/05/16 14:00), 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)
I checked the latest version from git and password change still does not work. There are also lots of other issues with ad provider (15 good, 35 bad) @see attached report.
BTW CI failed as well. ldap_local_override_test.py::test_regr_2790_override FAILED test_memory_cache.py::test_initgroups FAILED test_memory_cache.py::test_initgroups_with_mc FAILED test_memory_cache.py::test_initgroups_fqname_with_mc FAILED test_memory_cache.py::test_invalidation_of_gids_after_initgroups FAILED test_memory_cache.py::test_initgroups_without_change_in_membership FAILED test_memory_cache.py::test_invalidate_user_before_stop FAILED test_memory_cache.py::test_invalidate_user_after_stop FAILED test_memory_cache.py::test_invalidate_users_before_stop FAILED test_memory_cache.py::test_invalidate_users_after_stop FAILED test_memory_cache.py::test_invalidate_group_before_stop FAILED test_memory_cache.py::test_invalidate_group_after_stop FAILED test_memory_cache.py::test_invalidate_groups_before_stop FAILED
and make distcheck failed on rhel6 ../src/providers/ad/ad_init_new.c:42:40: error: providers/ad/ad_subdomains.h: No such file or directory ../src/providers/ad/ad_init_new.c: In function 'sssm_ad_subdomains_init': ../src/providers/ad/ad_init_new.c:623: error: implicit declaration of function 'ad_subdomains_init'
http://sssd-ci.duckdns.org/logs/job/43/95/summary.html
I am looking forward to the new fixed version :-)
LS
On Thu, May 26, 2016 at 11:31:07AM +0200, Lukas Slebodnik wrote:
On (16/05/16 14:00), 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)
I checked the latest version from git and password change still does not work. There are also lots of other issues with ad provider (15 good, 35 bad) @see attached report.
BTW CI failed as well. ldap_local_override_test.py::test_regr_2790_override FAILED test_memory_cache.py::test_initgroups FAILED test_memory_cache.py::test_initgroups_with_mc FAILED test_memory_cache.py::test_initgroups_fqname_with_mc FAILED test_memory_cache.py::test_invalidation_of_gids_after_initgroups FAILED test_memory_cache.py::test_initgroups_without_change_in_membership FAILED test_memory_cache.py::test_invalidate_user_before_stop FAILED test_memory_cache.py::test_invalidate_user_after_stop FAILED test_memory_cache.py::test_invalidate_users_before_stop FAILED test_memory_cache.py::test_invalidate_users_after_stop FAILED test_memory_cache.py::test_invalidate_group_before_stop FAILED test_memory_cache.py::test_invalidate_group_after_stop FAILED test_memory_cache.py::test_invalidate_groups_before_stop FAILED
and make distcheck failed on rhel6 ../src/providers/ad/ad_init_new.c:42:40: error: providers/ad/ad_subdomains.h: No such file or directory ../src/providers/ad/ad_init_new.c: In function 'sssm_ad_subdomains_init': ../src/providers/ad/ad_init_new.c:623: error: implicit declaration of function 'ad_subdomains_init'
http://sssd-ci.duckdns.org/logs/job/43/95/summary.html
I am looking forward to the new fixed version :-)
In addition, I was running downstream tests and even authentication was failing. I haven't had a chance to check LDAP authentication manually, though, I just ran the tests.
On 05/26/2016 11:47 AM, Jakub Hrozek wrote:
On Thu, May 26, 2016 at 11:31:07AM +0200, Lukas Slebodnik wrote:
On (16/05/16 14:00), 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)
I checked the latest version from git and password change still does not work. There are also lots of other issues with ad provider (15 good, 35 bad) @see attached report.
BTW CI failed as well. ldap_local_override_test.py::test_regr_2790_override FAILED test_memory_cache.py::test_initgroups FAILED test_memory_cache.py::test_initgroups_with_mc FAILED test_memory_cache.py::test_initgroups_fqname_with_mc FAILED test_memory_cache.py::test_invalidation_of_gids_after_initgroups FAILED test_memory_cache.py::test_initgroups_without_change_in_membership FAILED test_memory_cache.py::test_invalidate_user_before_stop FAILED test_memory_cache.py::test_invalidate_user_after_stop FAILED test_memory_cache.py::test_invalidate_users_before_stop FAILED test_memory_cache.py::test_invalidate_users_after_stop FAILED test_memory_cache.py::test_invalidate_group_before_stop FAILED test_memory_cache.py::test_invalidate_group_after_stop FAILED test_memory_cache.py::test_invalidate_groups_before_stop FAILED
and make distcheck failed on rhel6 ../src/providers/ad/ad_init_new.c:42:40: error: providers/ad/ad_subdomains.h: No such file or directory ../src/providers/ad/ad_init_new.c: In function 'sssm_ad_subdomains_init': ../src/providers/ad/ad_init_new.c:623: error: implicit declaration of function 'ad_subdomains_init'
http://sssd-ci.duckdns.org/logs/job/43/95/summary.html
I am looking forward to the new fixed version :-)
In addition, I was running downstream tests and even authentication was failing. I haven't had a chance to check LDAP authentication manually, though, I just ran the tests.
It works for me locally (even integration tests) so I suspect it will be one or small amount of bugs that repeats in those tests. Maybe some configuration that I did not tried. Logs and sssd.conf from some of those tests would be nice to have, otherwise I'm blind.
On (26/05/16 13:08), Pavel Březina wrote:
On 05/26/2016 11:47 AM, Jakub Hrozek wrote:
On Thu, May 26, 2016 at 11:31:07AM +0200, Lukas Slebodnik wrote:
On (16/05/16 14:00), 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)
I checked the latest version from git and password change still does not work. There are also lots of other issues with ad provider (15 good, 35 bad) @see attached report.
BTW CI failed as well. ldap_local_override_test.py::test_regr_2790_override FAILED
Most critical logs for this failure. The full domain log is in attachment. (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [sysdb_idmap_get_mappings] (0x0080): Could not locate ID mappings: [No such file or directory] (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_sudo_enabled] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work! (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_run_constructor] (0x0010): Unable to load target [selinux] constructor: /tmp/sssd-intg.MGrTl92m/lib/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_selinux_init (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_init] (0x0020): Target [selinux] is not supported by module [ldap]. (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_run_constructor] (0x0010): Unable to load target [hostid] constructor: /tmp/sssd-intg.MGrTl92m/lib/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_hostid_init (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_init] (0x0020): Target [hostid] is not supported by module [ldap]. (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_run_constructor] (0x0010): Unable to load target [subdomains] constructor: /tmp/sssd-intg.MGrTl92m/lib/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_subdomains_init (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_target_init] (0x0020): Target [subdomains] is not supported by module [ldap]. (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [dp_req_reply_gen_error] (0x0020): DP Request [Request Not Yet Created]: Finished. Error [1432158213]: DP target is not configured (Thu May 26 14:05:11 2016) [sssd[be[LDAP]]] [sbus_request_invoke_or_finish] (0x0020): Out of memory handling DBus message
Previous few lines sounds suspicious to me.
(Thu May 26 14:05:21 2016) [sssd[be[LDAP]]] [be_run_online_cb] (0x0080): Going online. Running callbacks. (Thu May 26 14:05:21 2016) [sssd[be[LDAP]]] [sdap_sudo_load_sudoers_done] (0x0040): Received 0 sudo rules (Thu May 26 14:05:21 2016) [sssd[be[LDAP]]] [be_ptask_enable] (0x0080): Task [SUDO Smart Refresh]: already enabled (Thu May 26 14:05:21 2016) [sssd[be[LDAP]]] [be_ptask_enable] (0x0080): Task [SUDO Full Refresh]: already enabled
LS
On 05/26/2016 02:11 PM, Lukas Slebodnik wrote:
On (26/05/16 13:08), Pavel Březina wrote:
On 05/26/2016 11:47 AM, Jakub Hrozek wrote:
On Thu, May 26, 2016 at 11:31:07AM +0200, Lukas Slebodnik wrote:
On (16/05/16 14:00), 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)
I just had a long debugging session with Jakub and we tracked the problem down to comparing uninitialized variable in create_initgr_ctx(). The diff is simple:
}
+ ret = EOK;
done: if (ret != EOK) { talloc_free(ctx); return NULL; }
return ctx; }
At least integration tests should work, downstream tests are in progress but I suppose it'll fix most of the issues if not all.
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.
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 }
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); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
and there are bunch of new warnings from static analyzers. Some of them might be caused by your obsession to move code here and back :-)
LS
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?
and there are bunch of new warnings from static analyzers. Some of them might be caused by your obsession to move code here and back :-)
I'm not really thrilled about those since it looks like the static analyzer went just mad. Almost all of those warning are about this:
ret = some_fn(&output) if (ret != EOK) { goto done; }
other_fn(output); ^ WARNING: output may be uninitialized
I consider this a false positive unless I miss something?
The other things are false positives as well, probably already marked as such. When we remove the _new suffix it will hopefully also get rid of it.
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.
and there are bunch of new warnings from static analyzers. Some of them might be caused by your obsession to move code here and back :-)
I'm not really thrilled about those since it looks like the static analyzer went just mad. Almost all of those warning are about this:
ret = some_fn(&output) if (ret != EOK) { goto done; }
other_fn(output); ^ WARNING: output may be uninitialized
I consider this a false positive unless I miss something?
The other things are false positives as well, probably already marked as such. When we remove the _new suffix it will hopefully also get rid of it.
Thank you for looking into this.
Thank you very much for your had work. I thing you deserve a huge steak from manager after this refactoring :-)
LS
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
LS
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
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.
LS
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?
} 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?
BTW test_dp_request fails with latest version. There are some leak report issues.
Thank you.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
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
On 06/16/2016 03:32 PM, Lukas Slebodnik wrote:
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 connectI 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?
On Thu, Jun 16, 2016 at 04:40:15PM +0200, Pavel Březina wrote:
On 06/16/2016 03:32 PM, Lukas Slebodnik wrote:
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 connectI 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?
nitpick: /sssd/src/providers/data_provider/dp_request_reply.c: In function ‘dp_req_with_reply_step’: /sssd/src/providers/data_provider/dp_request_reply.c:260: warning: declaration of ‘request_key’ shadows a global declaration /usr/include/keyutils.h:104: warning: shadowed declaration is here
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?
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
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.
There is a bug in forest user lookup when joined to child domain I verified that it works on master.
There is a current master installed on machine which I gave you. Try to copmare results with your branch
LS
On (17/06/16 16:54), 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.
There is a bug in forest user lookup when joined to child domain I verified that it works on master.
There is a current master installed on machine which I gave you. Try to copmare results with your branch
BTW your patch with braces in debug messages remind me to check missing new lines in your patches. @see attached diff
and please also clean test directories after successful tests. @see attached diff
LS
On 06/17/2016 08:33 PM, Lukas Slebodnik wrote:
On (17/06/16 16:54), 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.
There is a bug in forest user lookup when joined to child domain I verified that it works on master.
There is a current master installed on machine which I gave you. Try to copmare results with your branch
BTW your patch with braces in debug messages remind me to check missing new lines in your patches. @see attached diff
and please also clean test directories after successful tests. @see attached diff
LS
Thank you.
Diffs were squashed and forest user look up was fixed (I used basedn from sssd domain instead of forest).
I also removed suffix from new files and merged the content with the old ones. Since the review process is near the end and this way it will keep git blame and nicer history.
I added also rdp_message.c (rdp stands for resp. dp.) to simplify all communication with data provider from responders, now it's just pure tevent request. Later I'd like to extend it to support request grouping like we do today and create a more extensible and tevent-only dp interface for responders that would not depend on the output triplet (ret, dperror, msg) and allowed custom output data. But there's still time for this.
On Sat, Jun 18, 2016 at 04:11:30PM +0200, Pavel Březina wrote:
On 06/17/2016 08:33 PM, Lukas Slebodnik wrote:
On (17/06/16 16:54), 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.
There is a bug in forest user lookup when joined to child domain I verified that it works on master.
There is a current master installed on machine which I gave you. Try to copmare results with your branch
BTW your patch with braces in debug messages remind me to check missing new lines in your patches. @see attached diff
and please also clean test directories after successful tests. @see attached diff
LS
Thank you.
Diffs were squashed and forest user look up was fixed (I used basedn from sssd domain instead of forest).
I also removed suffix from new files and merged the content with the old ones. Since the review process is near the end and this way it will keep git blame and nicer history.
I added also rdp_message.c (rdp stands for resp. dp.) to simplify all communication with data provider from responders, now it's just pure tevent request. Later I'd like to extend it to support request grouping like we do today and create a more extensible and tevent-only dp interface for responders that would not depend on the output triplet (ret, dperror, msg) and allowed custom output data. But there's still time for this.
Thank you for the work on this patchset, this is a really nice improvement. I pushed all the patches to master: * a9aa70887985d37985093f1299fc15b2e060b2a0 * 83a79d93035c2d75a1941f3b54426119174044a0 * dea636af4d1902a081ee891f1b19ee2f8729d759 * 62370340092503baeaf6587d7ffe4fe25bd9582d * 49c467733ca65c9b77b9c33f38cdc223a99562e1 * b9d83e10cec267ae11fee64a30f42a12bbf7abe4 * 3e5e98aae4b14f5447c561ff5b0d854b74046312 * a1bf79449204ce9a5392b9d09b953a6bdf53a122 * d3dee2a07f1a8ee9ae6f94e149ced754ef76c248 * 565b9955cc439ade58cc24a98168060a60f33e7a * f48b1dda750511b57dce360fd18da2402adbcbb0 * 15f3e53e7ab9285afbebc8e2ec4d6d4577fef3da * dee7a89098b698e756f63e4041734d7322ad8b1e * c1058e96679c7ed1372825bf5226ce7d28a8e6ff * 75d66aea7accc842e68c88f085f9053112b20ecc * 4ebab24f65b54720a6672898b76185462015abab * 91c637615fc45679235b3371792e2eaf63a62b4f * d251d745f56bfd66e66dec9c646bae3003099f37 * 397bc52dd09a8c032abc7ea47a6d81dba5957464 * cc2d77d5218c188119fa954c856e858cbde76947 * 0429e21a45aa26c133cb4d8285a60446a0611e44 * 892ddeb5190dd5c1ffa26a95142a10a0034fc5e3 * 38b2bd97e41388995594126ea4e6b7c55ea0eb5c * 50c2a57dea6d38a4f6753a917a5d745b07036325 * 3b99f7a97553a0a357d50abe507d4f0060c4ecea
I moved one Makefile hunk in src/tests/cwrap/Makefile.am between the last two patches otherwise the patches wouldn't compile on their own.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina