I can reproduce with 24f850f on OpenBSD-current

eengler@openbsd:~/src/curl$ src/curl ldaps://db.debian.org/ 
DN: 
        objectClass: top
        objectClass: OpenLDAProotDSE


Segmentation fault (core dumped) 

I did some investigation on it and it turns out that the following line tries to access an item from a NULL pointer:

struct ldapconninfo *li = conn->proto.ldapc;

The problem is that conn (data->conn) is NULL. However data itself is set, just not conn.

I might find time to write a patch for it this weekend but I can't promise anything. Will post more updates regarding it soon.

I might find time to write a patch for it this weekend

I chased this down too and IMHO it's not as simple as it looks: data passed to ldap_disconnect is not the same as the one latched for ldapsb_tls. Daniel and me faced the same kind of problem in openssl while removing conn->data occurrences and we've made several unsuccessful trials before Daniel finally resolved it by a hack.

See #6518 (comment), #6446 (comment) and f2f91ac.

Okay I spent some time this morning inspecting it but I see myself unable to do it unfortunately. The problem is indeed much bigger

@bagder : thanks for fix. This effectively suppresses the segfault, but I wonder if the way it's done would'nt close the connection prematurely with regards to the LDAP protocol. I did not check, but the unbindRequest is probably not transmitted.

(This doesn't just suppress the segfault, it fixes the function to use the correct pointer and only use it if it is (still) valid.)

That's a slightly different but possibly still important issue: since the connection is already gone when ldap_disconnect is called the final actions are probably never done. I honestly don't know how important that is for users or servers.

The only proper way we can get this in order is to introduce an LDAP test server to the test suite and a few tests to make sure that curl speaks the protocol correctly. Easier said than done.

(This doesn't just suppress the segfault, it fixes the function to use the correct pointer and only use it if it is (still) valid.)

It seems you set the pointer to NULL: in ldap_disconnect:
ber_sockbuf_add_io(sb, &ldapsb_tls, LBER_SBIOD_LEVEL_TRANSPORT, NULL);
This disables effective io.

The only proper way we can get this in order is to introduce an LDAP test server to the test suite and a few tests to make sure that curl speaks the protocol correctly. Easier said than done.

I already dreamed of it: that's another story!

It seems you set the pointer to NULL: in ldap_disconnect:

Yes, because when we disconnect LDAP there is no longer any transfer associated to it...

Yes, because when we disconnect LDAP there is no longer any transfer associated to it...

Sure, but we're still having a (dummy?) data passed to curl_disconnect().

I had good results trying the following method:

• As you did for openssl trace, save data in the ldapconninfo new field io, revert the ldasb_tls stuff to use a conn pointer.
• In the ldapsb_tls_*() functions, use li->io as data (mimic conn->data).
• In ldap_disconnect(), save data->conn, set it to conn and set li->io to data.
• Restore data->conn before return.

Hacky-tricky, but it seems to work.

I can provide a PR if you like it.

Sure, but we're still having a (dummy?) data passed to curl_disconnect()

Curl_disconnect() gets the transfer passed in for which the associated connection should get disconnected. It's not a dummy.

But I was slightly wrong. The connection is actually still associated with the transfer when ldap_diconnect is called so it shouldn't have to be NULLed. I did that because the openldap callback got a data pointer to an already freed easy handle... I just draw the wrong conclusion. The correct fix there should be to update to the (new) transfer by updating the callback pointer to the new transfer:

diff --git a/lib/openldap.c b/lib/openldap.c
index 066c0fd73..c80ac8b07 100644
--- a/lib/openldap.c
+++ b/lib/openldap.c
@@ -369,11 +369,11 @@ static CURLcode ldap_disconnect(struct Curl_easy *data,
 
   if(li) {
     if(li->ld) {
       Sockbuf *sb;
       ldap_get_option(li->ld, LDAP_OPT_SOCKBUF, &sb);
-      ber_sockbuf_add_io(sb, &ldapsb_tls, LBER_SBIOD_LEVEL_TRANSPORT, NULL);
+      ber_sockbuf_add_io(sb, &ldapsb_tls, LBER_SBIOD_LEVEL_TRANSPORT, data);
       ldap_unbind_ext(li->ld, NULL, NULL);
       li->ld = NULL;
     }
     conn->proto.ldapc = NULL;
     free(li);

see #6787

I tried that alone, but it does not work, since data->conn is NULL.

For the original data, yes. This is not the same transfer object. The connection was moved to a new transfer.

For the original data, yes. This is not the same transfer object. The connection was moved to a new transfer.

Yes, you're right. I just tried #6787 with removals of null tests in tls io functions and it works.

I think we've finally got it. Thanks.