Learn more about these different git repos.
Other Git URLs
Talloc contains talloc_get_type_abort (const void *ptr,#type) which is a type safe equivalent to talloc_get_type(). The difference is that it calls an 'abort' function in case of type mismatch.
This 'abort' function can be set with: void talloc_set_abort_fn(void (abort_fn)(const char reason))
We should start using talloc_get_type_abort() with a custom 'abort' function, that would have two selectable behaviours:
Just to clarify, the idea would be to add a configure flag to choose between the two above behaviors. So developer builds could set --with-talloc-type-abort and released builds would default to a {{{SSSDBG_FATAL_FAILURE}}} error message at that point.
Fields changed
type: enhancement => task
milestone: NEEDS_TRIAGE => SSSD 1.9.0 NEEDS_TRIAGE
rhbz: => 0
milestone: SSSD 1.9.0 NEEDS_TRIAGE => NEEDS_TRIAGE
feature_milestone: => milestone: NEEDS_TRIAGE => SSSD 1.11.0 (LTM)
proposed_priority: => Optional
I think we should do this sooner. Implementing this checks would save devel time that was spent on a hard-to-spot bugs like #1416. Instead of a crash on NULL (and wondering how the heck could we get NULL there), we would get a nice abort message telling us the type we misused.
Clearing the evaluation tag.
proposed_priority: Optional => Undefined
proposed_priority: Undefined => Nice to have
Moving all the features planned for 1.10 release into 1.10 beta.
milestone: SSSD 1.11.0 (LTM) => SSSD 1.10 beta
priority: major => minor
I think we should abandon the idea of having a switch for how to handle talloc_get_type_abort().
We should definitely use this function, but always print a debug message describing the problem and abort the application if an error occurs for two reasons:
Fun facts:
selected: => Not need
Moving tickets that are not a priority for SSSD 1.10 into the next release.
milestone: SSSD 1.10 beta => SSSD 1.11 beta
mark: => 0
changelog: => design: => design_review: => 0 fedora_test_page: => milestone: SSSD 1.13 beta => SSSD 1.13 backlog review: => 0
Mass-moving tickets not planned for the next two releases.
Please reply with a comment if you disagree about the move..
milestone: SSSD 1.13 backlog => SSSD 1.15 beta
keywords: => easyfix sensitive: => 0
Hello,
Wanted to pick up this ticket. but seems does not have appropriate permissions.
Mentioned by jhrozek: You should see "Action" below "Change properties" and in action you should see "accept". But not able to find "Change Properties" button.
Thanks
owner: somebody => amitkumar25nov
Yes replacement of talloc_get_type() with talloc_get_type_abort() is good move, reasons mentioned in description. Current sssd contains 773 occurrences of talloc_get_type.
Fix is: Replacing all 773 occurrences with talloc_get_type_abort().
Sanity Test: a. # make intgchk b. Use case: Configuring AD trust using sssd.
My Queries: 1. Is Fix correct? 2. Does any other specific/corner test-case is required?
I would suggest to ask on the sssd list explicitly. Pavel Brezina might have a better idea
Replacing talloc_get_type() with talloc_get_type_abort() is only part of the patch, the other part would be providing talloc custom abort function that would tell us what has happened. I don't think any specific test case is required.
Queries from Comment23:
Huge Thanks In Advance Amit
_comment0: Queries from Comment23:
Huge Thanks In Advance Amit => 1483960104414237
When talloc_get_type_abort is called and the type of the talloc context does not match the desired type, talloc will call either abort() or a custom abort function if set. We want to provide this custom abort function that would print reason of the abort and then call abort(), so something like this:
void sss_talloc_abort(const char *reason) { DEBUG(SSSDBG_FATAL_FAILURE, "Talloc abort: %s\n", reason); abort(); }
Then you need to set this function with:
talloc_set_abort_fn(sss_talloc_abort);
See: https://talloc.samba.org/talloc/doc/html/group__talloc__debug.html#ga37273c95255f727c2e50be93ffe8d90d
Is it understandable?
These are changes I am planning: 1. changing all talloc_get_type() with talloc_get_type_abort() 2. Setting custom abort function //Declaration void sss_talloc_abort(const char reason); //util/util.h //Definition void sss_talloc_abort(const char reason){ //util/util.c DEBUG(SSSDBG_FATAL_FAILURE, "Talloc abort: %s\n", reason); abort(); } //Invocation int main(int argc, const char *argv[]){ //monitor/monitor.c talloc_set_abort_fn(sss_talloc_abort); } //Same from all separate processes int main() function:
/root/sssd/src ./external/inotify.m4:12:int main () { ./monitor/monitor.c:2507:int main(int argc, const char argv[]) ./p11_child/p11_child_nss.c:492:int main(int argc, const char argv[]) ./providers/ipa/selinux_child.c:191:int main(int argc, const char argv[]) ./providers/krb5/krb5_child.c:2690:int main(int argc, const char argv[]) ./providers/ldap/ldap_child.c:597:int main(int argc, const char argv[]) ./providers/proxy/proxy_child.c:503:int main(int argc, const char argv[]) ./providers/data_provider_be.c:486:int main(int argc, const char argv[]) ./responder/autofs/autofssrv.c:184:int main(int argc, const char argv[]) ./responder/ifp/ifpsrv.c:354:int main(int argc, const char argv[]) ./responder/nss/nsssrv.c:503:int main(int argc, const char argv[]) ./responder/pac/pacsrv.c:208:int main(int argc, const char argv[]) ./responder/pam/pamsrv.c:321:int main(int argc, const char argv[]) ./responder/secrets/secsrv.c:179:int main(int argc, const char argv[]) ./responder/ssh/sshsrv.c:176:int main(int argc, const char argv[]) ./responder/sudo/sudosrv.c:166:int main(int argc, const char argv[]) ./sss_client/autofs/autofs_test_client.c:39:int main(int argc, const char argv[]) ./sss_client/pam_test_client.c:51:int main(int argc, char argv[]) { ./sss_client/ssh/sss_ssh_authorizedkeys.c:31:int main(int argc, const char argv) ./sss_client/ssh/sss_ssh_knownhostsproxy.c:181:int main(int argc, const char argv) ./sss_client/sudo_testcli/sudo_testcli.c:39:int main(int argc, char argv) ./tests/ad_ldap_opt-tests.c:96:int main(void) ./tests/auth-tests.c:290:int main(int argc, const char argv[]) ./tests/check_and_open-tests.c:243:int main(void) ./tests/cmocka/data_provider/test_dp_request_table.c:308:int main(int argc, const char argv[]) ./tests/cmocka/data_provider/test_dp_request.c:411:int main(int argc, const char argv[]) ./tests/cmocka/data_provider/test_dp_builtin.c:139:int main(int argc, const char argv[]) ./tests/cmocka/dummy_child.c:33:int main(int argc, const char argv[]) ./tests/cmocka/sss_nss_idmap-tests.c:149:int main(int argc, const char argv[]) ./tests/cmocka/test_ad_access_filter.c:292:int main(int argc, const char argv[]) ./tests/cmocka/test_ad_gpo.c:332:int main(int argc, const char argv[]) ./tests/cmocka/test_authtok.c:562:int main(int argc, const char argv[]) ./tests/cmocka/test_cert_utils.c:371:int main(int argc, const char argv[]) ./tests/cmocka/test_copy_keytab.c:266:int main(int argc, const char argv[]) ./tests/cmocka/test_data_provider_be.c:202:int main(int argc, const char argv[]) ./tests/cmocka/test_dp_opts.c:466:int main(int argc, const char argv[]) ./tests/cmocka/test_dyndns.c:977:int main(int argc, const char argv[]) ./tests/cmocka/test_ad_subdomains.c:276:int main(int argc, const char argv[]) ./tests/cmocka/test_find_uid.c:96:int main(void) ./tests/cmocka/test_ifp.c:401:int main(int argc, const char argv[]) ./tests/cmocka/test_io.c:228:int main(void) ./tests/cmocka/test_ipa_dn.c:174:int main(int argc, const char argv[]) ./tests/cmocka/sbus_internal_tests.c:230:int main(int argc, const char argv[]) ./tests/cmocka/test_fqnames.c:464:int main(int argc, const char argv[]) ./tests/cmocka/test_krb5_wait_queue.c:316:int main(int argc, const char argv[]) ./tests/cmocka/test_child_common.c:503:int main(int argc, const char argv[]) ./tests/cmocka/test_ldap_id_cleanup.c:297:int main(int argc, const char argv[]) ./tests/cmocka/test_negcache.c:788:int main(void) ./tests/cmocka/test_pam_srv.c:1946:int main(int argc, const char argv[]) ./tests/cmocka/test_responder_cache_req.c:2095:int main(int argc, const char argv[]) ./tests/cmocka/test_sbus_opath.c:267:int main(int argc, const char argv[]) ./tests/cmocka/test_sdap.c:1122:int main(int argc, const char argv[]) ./tests/cmocka/test_search_bases.c:180:int main(void) ./tests/cmocka/test_sss_sifp.c:2121:int main(int argc, const char argv[]) ./tests/cmocka/test_sysdb_sudo.c:795:int main(int argc, const char argv[]) ./tests/cmocka/test_sysdb_ts_cache.c:1417:int main(int argc, const char argv[]) ./tests/cmocka/test_sysdb_utils.c:141:int main(int argc, const char argv[]) ./tests/cmocka/test_sysdb_views.c:1016:int main(int argc, const char argv[]) ./tests/cmocka/test_tools_colondb.c:356:int main(int argc, const char argv[]) ./tests/cmocka/test_utils.c:1864:int main(int argc, const char argv[]) ./tests/cmocka/test_ipa_idmap.c:215:int main(int argc, const char argv[]) ./tests/cmocka/test_sss_idmap.c:677:int main(int argc, const char argv[]) ./tests/cmocka/test_resolv_fake.c:360:int main(int argc, const char argv[]) ./tests/cmocka/test_simple_access.c:737:int main(int argc, const char argv[]) ./tests/cmocka/test_sdap_access.c:65:int main(void) ./tests/cmocka/test_sysdb_subdomains.c:619:int main(int argc, const char argv[]) ./tests/cmocka/test_nss_srv.c:3652:int main(int argc, const char argv[]) ./tests/cmocka/test_ipa_subdomains_utils.c:181:int main(int argc, const char argv[]) ./tests/cmocka/test_copy_ccache.c:200:int main(int argc, const char argv[]) ./tests/cmocka/test_responder_common.c:315:int main(int argc, const char argv[]) ./tests/cmocka/test_fo_srv.c:757:int main(int argc, const char argv[]) ./tests/cmocka/test_nested_groups.c:1279:int main(int argc, const char argv[]) ./tests/cmocka/test_ldap_auth.c:93:int main(void) ./tests/cmocka/test_ipa_subdomains_server.c:892:int main(int argc, const char argv[]) ./tests/cmocka/test_ad_common.c:883:int main(int argc, const char argv[]) ./tests/cmocka/test_krb5_common.c:249:int main(int argc, const char argv[]) ./tests/cmocka/test_be_ptask.c:966:int main(int argc, const char argv[]) ./tests/crypto-tests.c:226:int main(int argc, const char argv[]) ./tests/cwrap/test_become_user.c:128:int main(int argc, const char argv[]) ./tests/cwrap/test_negcache.c:653:int main(int argc, const char argv[]) ./tests/cwrap/test_server.c:164:int main(int argc, const char argv[]) ./tests/cwrap/test_usertools.c:70:int main(int argc, const char argv[]) ./tests/cwrap/test_responder_common.c:198:int main(int argc, const char argv[]) ./tests/debug-tests.c:677:int main(int argc, const char argv[]) ./tests/dlopen-tests.c:250:int main(int argc, const char argv[]) ./tests/files-tests.c:402:int main(int argc, const char argv[]) ./tests/find_uid-tests.c:115:int main(void) ./tests/ipa_hbac-tests.c:869:int main(int argc, const char argv[]) ./tests/ipa_ldap_opt-tests.c:508:int main(void) ./tests/krb5_utils-tests.c:756:int main(int argc, const char argv[]) ./tests/refcount-tests.c:195:int main(int argc, const char argv[]) ./tests/responder_socket_access-tests.c:139:int main(int argc, const char argv[]) ./tests/sbus_codegen_tests.c:1531:int main(int argc, const char argv[]) ./tests/sbus_tests.c:438:int main(int argc, const char argv[]) ./tests/sss_idmap-tests.c:955:int main(int argc, const char argv[]) ./tests/stress-tests.c:197:int main(int argc, const char argv[]) ./tests/strtonum-tests.c:576:int main(int argc, const char argv[]) { ./tests/sysdb-tests.c:6926:int main(int argc, const char argv[]) { ./tests/sysdb_ssh-tests.c:377:int main(int argc, const char argv[]) ./tests/util-tests.c:1178:int main(int argc, const char argv[]) ./tests/resolv-tests.c:992:int main(int argc, const char argv[]) ./tools/sss_cache.c:138:int main(int argc, const char *argv[]) ./tools/sss_debuglevel.c:58:int main(int argc, const char argv) ./tools/sss_groupadd.c:34:int main(int argc, const char argv) ./tools/sss_groupdel.c:33:int main(int argc, const char argv) ./tools/sss_groupmod.c:35:int main(int argc, const char argv) ./tools/sss_groupshow.c:654:int main(int argc, const char argv) ./tools/sss_override.c:1917:int main(int argc, const char argv) ./tools/sss_seed.c:777:int main(int argc, const char argv) ./tools/sss_signal.c:26:int main(int argc, const char argv) ./tools/sss_useradd.c:35:int main(int argc, const char argv) ./tools/sss_usermod.c:35:int main(int argc, const char argv) ./tools/sssctl/sssctl.c:260:int main(int argc, const char argv) ./tools/sss_userdel.c:120:int main(int argc, const char *argv)
We can plan to skip tests directory. But I feel it would be good to keep consistent approach for complete sssd(even on talloc_get_type_abort() failure)
Hello, Can you kindly review and provide comments? Thanks
The changes you are planning seems to be reasonable. I also agree with changing tests as well.
Metadata Update from @pbrezina: - Issue assigned to amitkumar25nov - Issue set to the milestone: SSSD Future releases (no date set yet)
PR:https://github.com/SSSD/sssd/pull/339
Metadata Update from @thalman: - Custom field design_review adjusted to on (was: 0) - Custom field mark adjusted to on (was: 0) - Custom field patch adjusted to on (was: 0) - Custom field review adjusted to on (was: 0) - Custom field sensitive adjusted to on (was: 0) - Custom field testsupdated adjusted to on (was: 0) - Issue close_status updated to: wontfix - Issue status updated to: Closed (was: Open)
SSSD is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in SSSD's github repository.
This issue has been cloned to Github and is available here: - https://github.com/SSSD/sssd/issues/2191
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Login to comment on this ticket.