#1149 Create custom abort function for talloc_get_type_abort()
Closed: wontfix 4 years ago by thalman. Opened 12 years ago by pbrezina.

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:

  • abort sssd so the developer can immediately notice the problem,
  • or print 'reason' DEBUG message.

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

Fields changed

milestone: NEEDS_TRIAGE => SSSD 1.9.0 NEEDS_TRIAGE

Fields changed

rhbz: => 0

Fields changed

milestone: SSSD 1.9.0 NEEDS_TRIAGE => NEEDS_TRIAGE

Fields changed

feature_milestone: =>
milestone: NEEDS_TRIAGE => SSSD 1.11.0 (LTM)

Fields changed

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

Fields changed

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

Fields changed

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:

  1. this is a fatal bug that should never go in master branch if the code is actually tried during devel and review process
  2. currently we almost never check the returned value from talloc_get_type(), it would require too many changes in the code that may bring additional problems

Fun facts:

  • if talloc_get_type() returns NULL than we will receive SIGSEGV later because we don't check returned value
  • we are already using talloc_get_type_abort() each time we use tevent_req_callback_data()

Fields changed

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

Fields changed

mark: => 0

Fields changed

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

Fields changed

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

Fields changed

owner: somebody => amitkumar25nov

Hello,

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?

Thanks

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:

  1. Replacing talloc_get_type() with talloc_get_type_abort() ==> OK
  2. Other part would be providing talloc custom abort function that would tell us what has happened ==> 3 Questions on this:
    a. You mean writing more talloc_asprintf() for better debug logs, then to be printed using talloc_log()?
    b. (if a is Yes)Should be part of same ticket?
    c. If (a,b are no). I am not able to get the point, can you kindly provide more descriptive/explaining text.

Huge Thanks In Advance
Amit

_comment0: Queries from Comment23:

  1. Replacing talloc_get_type() with talloc_get_type_abort() ==> OK
  2. Other part would be providing talloc custom abort function that would tell us what has happened ==> 2 Questions on this:
    a. You mean writing more talloc_asprintf() for better debug logs, then to be printed using talloc_log()?
    b. (if a is Yes)Should be part of same ticket?
    c. If (a,b are no). I am not able to get the point, can you kindly provide more descriptive/explaining text.

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?

Hello,

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:

pwd

/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)

Thanks

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)

7 years ago

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)

4 years ago

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.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata