[PATCH 0/1] MR3011: wldap32: Clean up LDAP_OPT_API_INFO.

[Alex Henrie (＠alexhenrie)]

Fix memory leaks on error paths, don't call memset unnecessarily, and
ensure that all output fields are initialized.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3011

[PATCH 1/1] wldap32: Clean up LDAP_OPT_API_INFO.

[Alex Henrie]

From: Alex Henrie <alexhenrie24(a)gmail.com>

Fix memory leaks on error paths, don't call memset unnecessarily, and
ensure that all output fields are initialized.
---
 dlls/wldap32/option.c | 72 +++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/dlls/wldap32/option.c b/dlls/wldap32/option.c
index 3d63b116e10..868c14d9211 100644
--- a/dlls/wldap32/option.c
+++ b/dlls/wldap32/option.c
@@ -63,27 +63,33 @@ ULONG CDECL ldap_get_optionA( LDAP *ld, int option, void *value )
         LDAPAPIInfoW infoW;
         LDAPAPIInfoA *infoA = value;
 
-        memset( &infoW, 0, sizeof(infoW) );
         infoW.ldapai_info_version = infoA->ldapai_info_version;
 
         ret = ldap_get_optionW( ld, option, &infoW );
-        if (ret == WLDAP32_LDAP_SUCCESS)
-        {
-            infoA->ldapai_api_version = infoW.ldapai_api_version;
-            infoA->ldapai_protocol_version = infoW.ldapai_protocol_version;
+        if (ret != WLDAP32_LDAP_SUCCESS) return ret;
 
-            if (infoW.ldapai_extensions && !(infoA->ldapai_extensions = strarrayWtoA( infoW.ldapai_extensions )))
-                return WLDAP32_LDAP_NO_MEMORY;
-            if (infoW.ldapai_vendor_name && !(infoA->ldapai_vendor_name = strWtoA( infoW.ldapai_vendor_name )))
-            {
-                ldap_value_freeW( infoW.ldapai_extensions );
-                return WLDAP32_LDAP_NO_MEMORY;
-            }
-            infoA->ldapai_vendor_version = infoW.ldapai_vendor_version;
+        infoA->ldapai_extensions = strarrayWtoA( infoW.ldapai_extensions );
+        if (infoW.ldapai_extensions && !infoA->ldapai_extensions)
+        {
+            ret = WLDAP32_LDAP_NO_MEMORY;
+            goto api_info_done;
+        }
 
-            ldap_value_freeW( infoW.ldapai_extensions );
-            ldap_memfreeW( infoW.ldapai_vendor_name );
+        infoA->ldapai_vendor_name = strWtoA( infoW.ldapai_vendor_name );
+        if (infoW.ldapai_vendor_name && !infoA->ldapai_vendor_name)
+        {
+            ldap_value_freeA( infoA->ldapai_extensions );
+            ret = WLDAP32_LDAP_NO_MEMORY;
+            goto api_info_done;
         }
+
+        infoA->ldapai_api_version = infoW.ldapai_api_version;
+        infoA->ldapai_protocol_version = infoW.ldapai_protocol_version;
+        infoA->ldapai_vendor_version = infoW.ldapai_vendor_version;
+
+api_info_done:
+        ldap_value_freeW( infoW.ldapai_extensions );
+        ldap_memfreeW( infoW.ldapai_vendor_name );
         return ret;
     }
 
@@ -184,27 +190,33 @@ ULONG CDECL ldap_get_optionW( LDAP *ld, int option, void *value )
         LDAPAPIInfo infoU;
         LDAPAPIInfoW *infoW = value;
 
-        memset( &infoU, 0, sizeof(infoU) );
         infoU.ldapai_info_version = infoW->ldapai_info_version;
 
         ret = map_error( ldap_get_option( CTX(ld), option, &infoU ) );
-        if (ret == WLDAP32_LDAP_SUCCESS)
-        {
-            infoW->ldapai_api_version = infoU.ldapai_api_version;
-            infoW->ldapai_protocol_version = infoU.ldapai_protocol_version;
+        if (ret != LDAP_SUCCESS) return ret;
 
-            if (infoU.ldapai_extensions && !(infoW->ldapai_extensions = strarrayUtoW( infoU.ldapai_extensions )))
-                return WLDAP32_LDAP_NO_MEMORY;
-            if (infoU.ldapai_vendor_name && !(infoW->ldapai_vendor_name = strUtoW( infoU.ldapai_vendor_name )))
-            {
-                ldap_memvfree( (void **)infoU.ldapai_extensions );
-                return WLDAP32_LDAP_NO_MEMORY;
-            }
-            infoW->ldapai_vendor_version = infoU.ldapai_vendor_version;
+        infoW->ldapai_extensions = strarrayUtoW( infoU.ldapai_extensions );
+        if (infoU.ldapai_extensions && !infoW->ldapai_extensions)
+        {
+            ret = WLDAP32_LDAP_NO_MEMORY;
+            goto api_info_done;
+        }
 
-            ldap_memvfree( (void **)infoU.ldapai_extensions );
-            ldap_memfree( infoU.ldapai_vendor_name );
+        infoW->ldapai_vendor_name = strUtoW( infoU.ldapai_vendor_name );
+        if (infoU.ldapai_vendor_name && !infoW->ldapai_vendor_name)
+        {
+            ret = WLDAP32_LDAP_NO_MEMORY;
+            strarrayfreeW( infoW->ldapai_extensions );
+            goto api_info_done;
         }
+
+        infoW->ldapai_api_version = infoU.ldapai_api_version;
+        infoW->ldapai_protocol_version = infoU.ldapai_protocol_version;
+        infoW->ldapai_vendor_version = infoU.ldapai_vendor_version;
+
+api_info_done:
+        ldap_memvfree( (void **)infoU.ldapai_extensions );
+        ldap_memfree( infoU.ldapai_vendor_name );
         return ret;
     }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/3011

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/wldap32/option.c:
> LDAPAPIInfo infoU;
>          LDAPAPIInfoW *infoW = value;
>
> -        memset( &infoU, 0, sizeof(infoU) );
>          infoU.ldapai_info_version = infoW->ldapai_info_version;
>
> ret = map_error( ldap_get_option( CTX(ld), option, &infoU ) );
> -        if (ret == WLDAP32_LDAP_SUCCESS)
> -        {
> -            infoW->ldapai_api_version = infoU.ldapai_api_version;
> -            infoW->ldapai_protocol_version = infoU.ldapai_protocol_version;
> +        if (ret != LDAP_SUCCESS) return ret;

Please keep WLDAP32_LDAP_SUCCESS. It has the same value as LDAP_SUCCESS but map_error() returns WLDAP32_ prefixed values.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3011#note_35091

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/wldap32/option.c:
> -            ldap_memfree( infoU.ldapai_vendor_name );
> +        infoW->ldapai_vendor_name = strUtoW( infoU.ldapai_vendor_name );
> +        if (infoU.ldapai_vendor_name && !infoW->ldapai_vendor_name)
> +        {
> +            ret = WLDAP32_LDAP_NO_MEMORY;
> +            strarrayfreeW( infoW->ldapai_extensions );
> +            goto api_info_done;
>          }
> +
> +        infoW->ldapai_api_version = infoU.ldapai_api_version;
> +        infoW->ldapai_protocol_version = infoU.ldapai_protocol_version;
> +        infoW->ldapai_vendor_version = infoU.ldapai_vendor_version;
> +
> +api_info_done:
> +        ldap_memvfree( (void **)infoU.ldapai_extensions );
> +        ldap_memfree( infoU.ldapai_vendor_name );
> I'd prefer not to use a goto here. It will be shorter too. Let's use strarrayfreeU/W and free instead of ldap_memvfree/ldap_value_freeW and ldap_memfree/W.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3011#note_35092

[Hans Leidekker (＠hans)]

On Thu Jun  8 08:22:38 2023 +0000, Hans Leidekker wrote:
> I'd prefer not to use a goto here. It will be shorter too. Let's use
> strarrayfreeU/W and free instead of ldap_memvfree/ldap_value_freeW and ldap_memfree/W.
> s/strarrayfreeU/strarrayfreeA/

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3011#note_35100