[PATCH v2 0/1] MR2481: wldap32: Handle null DN or null message in ldap_delete* and add tests.
On Windows, ldap_delete_(ext_)s returns LDAP_UNWILLING_TO_PERFORM when trying to delete an entry that cannot be deleted. On Unix, LDAP_SERVER_DOWN is returned instead. Unix also returns LDAP_SERVER_DOWN immediately from ldap_delete_ext if the entry cannot be deleted, whereas Windows queues an asynchronous operation and returns LDAP_SUCCESS. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54702 -- v2: wldap32: Handle null DN or null message in ldap_delete* and add tests. https://gitlab.winehq.org/wine/wine/-/merge_requests/2481
From: Alex Henrie <alexhenrie24(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54702 --- dlls/wldap32/delete.c | 8 +++--- dlls/wldap32/tests/parse.c | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/dlls/wldap32/delete.c b/dlls/wldap32/delete.c index b085142d83d..8539b7266eb 100644 --- a/dlls/wldap32/delete.c +++ b/dlls/wldap32/delete.c @@ -72,7 +72,7 @@ ULONG CDECL ldap_delete_extA( LDAP *ld, char *dn, LDAPControlA **serverctrls, LD TRACE( "(%p, %s, %p, %p, %p)\n", ld, debugstr_a(dn), serverctrls, clientctrls, message ); - if (!ld) return WLDAP32_LDAP_PARAM_ERROR; + if (!ld || !message) return WLDAP32_LDAP_PARAM_ERROR; if (dn && !(dnW = strAtoW( dn ))) goto exit; if (serverctrls && !(serverctrlsW = controlarrayAtoW( serverctrls ))) goto exit; @@ -99,9 +99,9 @@ ULONG CDECL ldap_delete_extW( LDAP *ld, WCHAR *dn, LDAPControlW **serverctrls, L TRACE( "(%p, %s, %p, %p, %p)\n", ld, debugstr_w(dn), serverctrls, clientctrls, message ); - if (!ld) return WLDAP32_LDAP_PARAM_ERROR; + if (!ld || !message) return WLDAP32_LDAP_PARAM_ERROR; - if (dn && !(dnU = strWtoU( dn ))) goto exit; + if (!(dnU = dn ? strWtoU( dn ) : strdup( "" ))) goto exit; if (serverctrls && !(serverctrlsU = controlarrayWtoU( serverctrls ))) goto exit; if (clientctrls && !(clientctrlsU = controlarrayWtoU( clientctrls ))) goto exit; else @@ -155,7 +155,7 @@ ULONG CDECL ldap_delete_ext_sW( LDAP *ld, WCHAR *dn, LDAPControlW **serverctrls, if (!ld) return WLDAP32_LDAP_PARAM_ERROR; - if (dn && !(dnU = strWtoU( dn ))) goto exit; + if (!(dnU = dn ? strWtoU( dn ) : strdup( "" ))) goto exit; if (serverctrls && !(serverctrlsU = controlarrayWtoU( serverctrls ))) goto exit; if (clientctrls && !(clientctrlsU = controlarrayWtoU( clientctrls ))) goto exit; else diff --git a/dlls/wldap32/tests/parse.c b/dlls/wldap32/tests/parse.c index 34fcb9a4201..453ab7ce27c 100644 --- a/dlls/wldap32/tests/parse.c +++ b/dlls/wldap32/tests/parse.c @@ -367,6 +367,55 @@ static void test_ldap_compare(void) ldap_unbind( ld ); } +static void test_ldap_delete(void) +{ + LDAP *ld; + ULONG ret, num; + + ld = ldap_initA( (char *)"db.debian.org", 389 ); + ok( ld != NULL, "ldap_init failed\n" ); + + ret = ldap_deleteA( NULL, NULL ); + ok( ret == (ULONG)-1, "ldap_deleteA should fail, got %#lx\n", ret ); + ret = ldap_deleteA( NULL, (char *)"" ); + ok( ret == (ULONG)-1, "ldap_deleteA should fail, got %#lx\n", ret ); + ret = ldap_deleteA( ld, NULL ); + ok( ret != (ULONG)-1, "ldap_deleteA should succeed, got %#lx\n", ret ); + ret = ldap_deleteA( ld, (char *)"" ); + ok( ret != (ULONG)-1, "ldap_deleteA should succeed, got %#lx\n", ret ); + + ret = ldap_delete_sA( NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_sA( NULL, (char *)"" ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_sA( ld, NULL ); + flaky_wine ok( ret == LDAP_UNWILLING_TO_PERFORM, "ldap_delete_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_sA( ld, (char *)"" ); + flaky_wine ok( ret == LDAP_UNWILLING_TO_PERFORM, "ldap_delete_sA should fail, got %#lx\n", ret ); + + ret = ldap_delete_extA( NULL, NULL, NULL, NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_extA should fail, got %#lx\n", ret ); + ret = ldap_delete_extA( NULL, (char *)"", NULL, NULL, &num ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_extA should fail, got %#lx\n", ret ); + ret = ldap_delete_extA( ld, NULL, NULL, NULL, &num ); + flaky_wine ok( !ret, "ldap_delete_extA should succeed, got %#lx\n", ret ); + ret = ldap_delete_extA( ld, (char *)"", NULL, NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_extA should fail, got %#lx\n", ret ); + ret = ldap_delete_extA( ld, (char *)"", NULL, NULL, &num ); + flaky_wine ok( !ret, "ldap_delete_extA should succeed, got %#lx\n", ret ); + + ret = ldap_delete_ext_sA( NULL, NULL, NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_ext_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_ext_sA( NULL, (char *)"", NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_ext_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_ext_sA( ld, NULL, NULL, NULL ); + flaky_wine ok( ret == LDAP_UNWILLING_TO_PERFORM, "ldap_delete_ext_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_ext_sA( ld, (char *)"", NULL, NULL ); + flaky_wine ok( ret == LDAP_UNWILLING_TO_PERFORM, "ldap_delete_ext_sA should fail, got %#lx\n", ret ); + + ldap_unbind( ld ); +} + static void test_ldap_server_control( void ) { /* SEQUENCE { INTEGER :: 0x07 } */ @@ -492,6 +541,7 @@ START_TEST (parse) test_ldap_add(); test_ldap_modify(); test_ldap_compare(); + test_ldap_delete(); ld = ldap_initA( (char *)"db.debian.org", 389 ); ok( ld != NULL, "ldap_init failed\n" ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2481
Hans Leidekker (@hans) commented about dlls/wldap32/tests/parse.c:
+ + ret = ldap_deleteA( NULL, NULL ); + ok( ret == (ULONG)-1, "ldap_deleteA should fail, got %#lx\n", ret ); + ret = ldap_deleteA( NULL, (char *)"" ); + ok( ret == (ULONG)-1, "ldap_deleteA should fail, got %#lx\n", ret ); + ret = ldap_deleteA( ld, NULL ); + ok( ret != (ULONG)-1, "ldap_deleteA should succeed, got %#lx\n", ret ); + ret = ldap_deleteA( ld, (char *)"" ); + ok( ret != (ULONG)-1, "ldap_deleteA should succeed, got %#lx\n", ret ); + + ret = ldap_delete_sA( NULL, NULL ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_sA( NULL, (char *)"" ); + ok( ret == LDAP_PARAM_ERROR, "ldap_delete_sA should fail, got %#lx\n", ret ); + ret = ldap_delete_sA( ld, NULL ); + flaky_wine ok( ret == LDAP_UNWILLING_TO_PERFORM, "ldap_delete_sA should fail, got %#lx\n", ret ); I don't think we want to introduce flaky tests for these edge cases. If there's an application that depends on this then it would be better to fix Wine/OpenLDAP.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2481#note_27612
Hans Leidekker (@hans) commented about dlls/wldap32/tests/parse.c:
ldap_unbind( ld ); }
+static void test_ldap_delete(void) +{ + LDAP *ld; + ULONG ret, num; + + ld = ldap_initA( (char *)"db.debian.org", 389 ); + ok( ld != NULL, "ldap_init failed\n" );
Please reuse the existing connection like in test_ldap_parse_sort_control() so we don't put unnecessary burden on the server. Same goes for the tests you added in previous commits. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2481#note_27613
Those tests are special because they use a specific option or a secure connection. Let's leave them as they are. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2481#note_27650
On Thu Mar 23 08:40:41 2023 +0000, Hans Leidekker wrote:
Please reuse the existing connection like in test_ldap_parse_sort_control() so we don't put unnecessary burden on the server. Same goes for the tests you added in previous commits. OK, so don't call ldap_init in test_ldap_(add|modify|compare|delete)? Do you want me to remove the calls to ldap_init and ldap_sslinit from test_ldap_(paged_search|server_control|bind_sA) as well?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2481#note_27644
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Hans Leidekker (＠hans)