https://bugs.openldap.org/show_bug.cgi?id=9530
Issue ID: 9530 Summary: double-free in options.c Product: OpenLDAP Version: 2.4.58 Hardware: All OS: All Status: UNCONFIRMED Severity: normal Priority: --- Component: libraries Assignee: bugs@openldap.org Reporter: norm.green@gemtalksystems.com Target Milestone: ---
I've been seeing double-free errors in valgrind when calling
ldap_set_option(lc, LDAP_OPT_DEFBASE)
I tracked it down to code in ldap_create() in open.c. When we copy the global options to the new LDAP *, we create new versions of some but not all malloced options. The ldo_defbase and ldo_defbinddn option members are strings that are *not* reallocated (ldo_defbase may not be important).
This diff appears to fix the problem:
diff --git a/libraries/libldap/open.c b/libraries/libldap/open.c index 5882b6336..0828d334e 100644 --- a/libraries/libldap/open.c +++ b/libraries/libldap/open.c @@ -139,6 +139,14 @@ ldap_create( LDAP **ldp ) ld->ld_options.ldo_defludp = NULL; ld->ld_options.ldo_conn_cbs = NULL;
+ /* Norm Green, April 20, 2021 - fix pointers that get copied. + * must realloc these to prevent double-free errors */ + + ld->ld_options.ldo_defbase = gopts->ldo_defbase ? + LDAP_STRDUP(gopts->ldo_defbase) : NULL; + ld->ld_options.ldo_defbinddn = gopts->ldo_defbinddn ? + LDAP_STRDUP(gopts->ldo_defbinddn) : NULL; +
https://bugs.openldap.org/show_bug.cgi?id=9530
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |needs_review
https://bugs.openldap.org/show_bug.cgi?id=9530
--- Comment #1 from Howard Chu hyc@openldap.org --- (In reply to norm.green@gemtalksystems.com from comment #0)
I've been seeing double-free errors in valgrind when calling
ldap_set_option(lc, LDAP_OPT_DEFBASE)
I tracked it down to code in ldap_create() in open.c. When we copy the global options to the new LDAP *, we create new versions of some but not all malloced options. The ldo_defbase and ldo_defbinddn option members are strings that are *not* reallocated (ldo_defbase may not be important).
This diff appears to fix the problem:
diff --git a/libraries/libldap/open.c b/libraries/libldap/open.c index 5882b6336..0828d334e 100644 --- a/libraries/libldap/open.c +++ b/libraries/libldap/open.c @@ -139,6 +139,14 @@ ldap_create( LDAP **ldp ) ld->ld_options.ldo_defludp = NULL; ld->ld_options.ldo_conn_cbs = NULL;
- /* Norm Green, April 20, 2021 - fix pointers that get copied.
* must realloc these to prevent double-free errors */
- ld->ld_options.ldo_defbase = gopts->ldo_defbase ?
LDAP_STRDUP(gopts->ldo_defbase) : NULL;
That appears to be correct.
- ld->ld_options.ldo_defbinddn = gopts->ldo_defbinddn ?
LDAP_STRDUP(gopts->ldo_defbinddn) : NULL;
This appears to be unnecessary, since there are no functions to modify ldo_defbinddn after initialization.
https://bugs.openldap.org/show_bug.cgi?id=9530
--- Comment #2 from norm.green@gemtalksystems.com norm.green@gemtalksystems.com ---
This appears to be unnecessary, since there are no functions to modify >ldo_defbinddn after initialization.
Agreed, however the old code is still copying a string pointer to a new location, which is asking for trouble should the pointer is freed in both locations.
Norm Green
https://bugs.openldap.org/show_bug.cgi?id=9530
--- Comment #3 from Howard Chu hyc@openldap.org --- (In reply to norm.green@gemtalksystems.com from comment #2)
This appears to be unnecessary, since there are no functions to modify >ldo_defbinddn after initialization.
Agreed, however the old code is still copying a string pointer to a new location, which is asking for trouble should the pointer is freed in both locations.
There is no such code that ever frees this field. That could be considered a bug in itself, but it's a different bug. While it may be a memory leak we generally don't care about leaks of values that are allocated only once at initialization time and never touched again.
https://bugs.openldap.org/show_bug.cgi?id=9530
Howard Chu hyc@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |TEST
--- Comment #4 from Howard Chu hyc@openldap.org --- fixed in master
https://bugs.openldap.org/show_bug.cgi?id=9530
--- Comment #5 from Quanah Gibson-Mount quanah@openldap.org --- Commits: • 87397b34 by Norm Green at 2021-04-21T18:03:43+01:00 ITS#9530 fix double-free of LDAP_OPT_DEFBASE
Commits: • edfc4e7f by Howard Chu at 2021-04-21T18:06:26+01:00 ITS#9530 ldo_defbase now must be freed in ldap_ld_free()
https://bugs.openldap.org/show_bug.cgi?id=9530
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords|needs_review | Target Milestone|--- |2.4.59
https://bugs.openldap.org/show_bug.cgi?id=9530
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|TEST |FIXED
--- Comment #6 from Quanah Gibson-Mount quanah@openldap.org --- RE24:
commit 5452fb154eaabc46992402a4f1bb1081bc367157 Author: Howard Chu hyc@openldap.org Date: Wed Apr 21 18:06:26 2021 +0100
ITS#9530 ldo_defbase now must be freed in ldap_ld_free()
commit 32e965c27134d3d83adbf471186330e361150491 Author: Norm Green norm.green@gemtalksystems.com Date: Wed Apr 21 04:35:30 2021 +0000
ITS#9530 fix double-free of LDAP_OPT_DEFBASE
https://bugs.openldap.org/show_bug.cgi?id=9530
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |VERIFIED