When a cyrus-sasl client provides a SASL_CB_PASS callback to supply passwords to a sasl_client_step process, it is expected to allocate a new sasl_secret_t and return it in the fourth parameter of the callback function. However, _plug_get_password fails to mark that passed-through newly-allocated object as *iscopy=1, leading to a memory leak. This appears as recently as f21/rawhide's cyrus-sasl-2.1.26, and probably earlier. Specifically: int _plug_get_password(const sasl_utils_t *utils, sasl_secret_t **password, unsigned int *iscopy, sasl_interact_t **prompt_need) { [...] *password = NULL; *iscopy = 0; [...] /* Try to get the callback... */ ret = utils->getcallback(utils->conn, SASL_CB_PASS, (sasl_callback_ft *)&pass_cb, &pass_context); if (ret == SASL_OK && pass_cb) { ret = pass_cb(utils->conn, pass_context, SASL_CB_PASS, password); if (ret != SASL_OK) return ret; if (!*password) { PARAMERROR(utils); return SASL_BADPARAM; } } return ret; } Note *iscopy is left at 0, which plugins/digestmd5.c's ask_user_info interprets to mean that the object doesn't need to be free()d.
Hi Frank, can you provide me some example with minimal reproducer, to see how does it work? Yes, there can be problems, but it would be good have some example where it should work and it doesn't and to verify the results.
A reproducer would be installing pcp, starting the pmcd service, then running valgrind --leak-check=full pminfo -h 'pcp://localhost?user=NOsuchUSER&pass=DEFINITELYnotApassword' The memory leak will be shown.
Created attachment 997766 [details] Mark returned secret as a copy even if it is returned from callback to achieve correct free Thank you for reporting. This is also reproducible across all our releases from f20 to rawhide. ==1624== 39 bytes in 1 blocks are definitely lost in loss record 14 of 37 ==1624== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1624== by 0x4E8582A: __pmAuthSecretCB (secureconnect.c:750) ==1624== by 0x5C235CA: _plug_get_password (plugin_common.c:411) ==1624== by 0x9916450: ask_user_info (digestmd5.c:4084) [...] This issue can be fixed by specifying *iscopy = 1; after successful callback return (attached patch). But using pcp-3.10.2 there is still one invalid free in this context: ==1625== Invalid free() / delete / delete[] / realloc() ==1625== at 0x4C28577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1625== by 0x4E66D81: attrHashNodeDel (spec.c:682) ==1625== by 0x4E7C054: __pmHashWalkCB (hash.c:146) ==1625== by 0x4E45958: pmNewContext (context.c:518) ==1625== by 0x4017F0: main (pminfo.c:642) ==1625== Address 0x7cda5d0 is 0 bytes inside a block of size 23 free'd ==1625== at 0x4C28577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1625== by 0x4E85858: __pmAuthSecretCB (secureconnect.c:764) ==1625== by 0x5C235CA: _plug_get_password (plugin_common.c:411) which is probably fixed by pcp upstream commit: http://oss.sgi.com/cgi-bin/gitweb.cgi?p=pcp/pcp.git;a=commitdiff;h=4670799bf275679fe4030d72f10003f7e15378fa With pcp-3.10.3 and this patch I got "empty" list except ==21520== still reachable: 143,843 bytes in 215 blocks I hope this is not a regression against other tools using this library. I will discuss it with Petr.
cyrus-sasl-2.1.26-21.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/cyrus-sasl-2.1.26-21.fc22
Package cyrus-sasl-2.1.26-21.fc22: * should fix your issue, * was pushed to the Fedora 22 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing cyrus-sasl-2.1.26-21.fc22' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-2015-3857/cyrus-sasl-2.1.26-21.fc22 then log in and leave karma (feedback).
Seems like this is not as easy as it looked like. Some other tools are freeing this memory on their own (subversion) and this fix made them fail (related bz1202364). According to sasl programming guide, "whoever allocates the memory is responsible for freeing it" [1], so this is not a bug in cyrus-sasl, but in pminfo. Reverting commit and reassigning to pcp component. [1] http://www.sendmail.org/~ca/email/cyrus2/programming.html#callbacks_interactions
It appears as though the memory ownership situation with this aspect of sasl is inconsistent enough that some applications (like sendmail; see usersmtp.c:1138 getsecret) tolerate memory leaks; others (like cvs) assume the current behavior and do a deferred free(). If PCP adopts the second model, we'll have to audit cyrus-sasl to check which of them it frees vs. doesn't. For the latter, we'd have to track all these heap-allocated strings from the various callbacks, and free them after the sasl conversation is finished.
Package cyrus-sasl-2.1.26-22.fc22: * should fix your issue, * was pushed to the Fedora 22 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing cyrus-sasl-2.1.26-22.fc22' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-2015-3857/cyrus-sasl-2.1.26-22.fc22 then log in and leave karma (feedback).
cyrus-sasl-2.1.26-22.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
This message is a reminder that Fedora 21 is nearing its end of life. Approximately 4 (four) weeks from now Fedora will stop maintaining and issuing updates for Fedora 21. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a Fedora 'version' of '21'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, simply change the 'version' to a later Fedora version. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora 21 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora, you are encouraged change the 'version' to a later Fedora version prior this bug is closed as described in the policy above. Although we aim to fix as many bugs as possible during every release's lifetime, sometimes those efforts are overtaken by events. Often a more recent Fedora release includes newer upstream software that fixes bugs or makes them obsolete.
Fedora 21 changed to end-of-life (EOL) status on 2015-12-01. Fedora 21 is no longer maintained, which means that it will not receive any further security or bug fix updates. As a result we are closing this bug. If you can reproduce this bug against a currently maintained version of Fedora please feel free to reopen this bug against that version. If you are unable to reopen this bug, please file a new report against the current release. If you experience problems, please add a comment to this bug. Thank you for reporting this bug and we are sorry it could not be fixed.