1) if the given dn is "", that 1 byte will be leaked when the dn is reassigned to the bind dn 2) calling slapi_pblock_get with SLAPI_CONN_DN does a strdup, which is different than most uses of slapi_pblock_get. That memory must be freed. 3) If we set the ORIGINAL_TARGET to a dn other than the given dn, we must free it
Created attachment 313861 [details] diffs
(In reply to comment #0) > 3) If we set the ORIGINAL_TARGET to a dn other than the given dn, we must free > it It looks to me that when a dn other than the given dn is set to ORIGINAL_TARGET, the given dn (dn == "") is already freed by your fix. And the dn set to ORIGINAL_TARGET is the one duplicated from "bindDN", which is freed at the line 753. But I should be missing something. Are there any place "dn" is overwritten after setting to ORIGINAL_TARGET? 660 /* Determine the target DN for this operation */ 661 /* Did they give us a DN ? */ 662 if (dn == NULL || *dn == '\0') { 663 /* Get the DN from the bind identity on this connection */ slapi_ch_free_string(&dn); 664 dn = slapi_ch_strdup(bindDN); 665 LDAPDebug( LDAP_DEBUG_ANY, 666 "Missing userIdentity in request, using the bind DN inst ead.\n", 667 0, 0, 0 ); 668 } 669 670 slapi_pblock_set( pb, SLAPI_ORIGINAL_TARGET, dn ); 671 749 /* Either this is the same pointer that we allocated and set above, 750 * or whoever used it should have freed it and allocated a new 751 * value that we need to free here */ 752 slapi_pblock_get( pb, SLAPI_ORIGINAL_TARGET, &dn ); 753 slapi_ch_free_string(&dn); 754 slapi_pblock_set( pb, SLAPI_ORIGINAL_TARGET, NULL );
(In reply to comment #2) > (In reply to comment #0) > > > 3) If we set the ORIGINAL_TARGET to a dn other than the given dn, we must free > > it > > It looks to me that when a dn other than the given dn is set to > ORIGINAL_TARGET, the given dn (dn == "") is already freed by your fix. Not necessarily. There are several error conditions earlier in the function that can cause the code to jump to free_and_return: after dn has been allocated. If this happens, we must free dn. > And the > dn set to ORIGINAL_TARGET is the one duplicated from "bindDN", which is freed > at the line 753. But I should be missing something. Are there any place "dn" > is overwritten after setting to ORIGINAL_TARGET? > 660 /* Determine the target DN for this operation */ > 661 /* Did they give us a DN ? */ > 662 if (dn == NULL || *dn == '\0') { > 663 /* Get the DN from the bind identity on this connection */ > slapi_ch_free_string(&dn); > 664 dn = slapi_ch_strdup(bindDN); > 665 LDAPDebug( LDAP_DEBUG_ANY, > 666 "Missing userIdentity in request, using the bind DN > inst ead.\n", > 667 0, 0, 0 ); > 668 } > 669 > 670 slapi_pblock_set( pb, SLAPI_ORIGINAL_TARGET, dn ); > 671 > > 749 /* Either this is the same pointer that we allocated and set above, > 750 * or whoever used it should have freed it and allocated a new > 751 * value that we need to free here */ > 752 slapi_pblock_get( pb, SLAPI_ORIGINAL_TARGET, &dn ); > 753 slapi_ch_free_string(&dn); > 754 slapi_pblock_set( pb, SLAPI_ORIGINAL_TARGET, NULL );
The fix looks fine, but I am also curious about Noriko's second question. In what cases is SLAPI_ORIGINAL_TARGET changed to something other than the "dn" pointer?
This leak can be triggered by an anonymous user. There is no way I know of to mitigate this issue.
Created attachment 314146 [details] cvs commit log - DS8.0 Reviewed by: nkinder (Thanks!) Fix Description: 1) if the given dn is "", that 1 byte will be leaked when the dn is reassigned to the bind dn - so free it first in that case before reassigning 2) calling slapi_pblock_get with SLAPI_CONN_DN does a strdup, which is different than most uses of slapi_pblock_get. That memory must be freed. So we free it at the end. 3) If we set the ORIGINAL_TARGET to a dn other than the given dn, we must free it - grab it and compare it to dn - if not the same, free dn first, then free the original target dn Platforms tested: RHEL5, Fedora 8 Flag Day: no Doc impact: no QA impact: should be covered by regular nightly and manual testing New Tests integrated into TET: none
How can QE verify this? What to look for in the valgrind output?
(In reply to comment #8) > How can QE verify this? What to look for in the valgrind output? Look for a memory leak in passwd_modify_extop()
verified 8.0 RHEL4-32, RHEL4-64, RHEL5-32, RHEL5-64
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHSA-2008-0602.html
Created attachment 315146 [details] cvs commit log - HEAD