Skip to content

ldap.initialize(fileno=fileno) does take ownership of the fileno #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ClydeByrdIII opened this issue Aug 30, 2024 · 1 comment
Open

ldap.initialize(fileno=fileno) does take ownership of the fileno #575

ClydeByrdIII opened this issue Aug 30, 2024 · 1 comment

Comments

@ClydeByrdIII
Copy link

ClydeByrdIII commented Aug 30, 2024 •
edited
Loading

Issue description:

The ldap.initialize() documents say

If fileno parameter is given then the file descriptor will be used to connect to an LDAP server. The fileno must either be a socket file descriptor as int or a file-like object with a fileno() method that returns a socket file descriptor. The socket file descriptor must already be connected. LDAPObject does not take ownership of the file descriptor. It must be kept open during operations and explicitly closed after the LDAPObject is unbound.

However it seemingly is the case that it does take ownership

1490e99, #460, #543 are likely due to this confusion as well.

Following the code shows it goes like this.

ldap.initalize(*args, fileno=fd) will result in ldap_init_fd() being called which executes code that essentially creates a new ldap connection object that is then bound to the passed in fileno it then based on the protocol passed (LDAP_PROTO_TCP, LDAP_PROTO_UDP, LDAP_PROTO_IPC, LDAP_PROTO_EXT) will assign the protocols respective sockbuf_IO(s) set of {setup, ctrl, read, write, close} functions (except for LDAP_PROTO_EXT which the user specifies a custom struct elsewhere). These functions dictate how to interact with the underlying communication mechanism. Most relevant to this issue is the “close” functions

For TCP/UDP sockbuf_IO it calls “tcp_close()” which is system dependent macro but for Linux is likely the below

define tcp_close( s ) (shutdown( s, SHUT_RDWR ), close( s ))

For IPC it uses ber_sockbuf_io_fd (where sb_fd_close() is just close(2)

In all cases except LDAP_PROTO_EXT(where it’s user dependent), when a connection is freed some form of close() is called on the fd.

I’m not sure why the documentation thinks it doesn’t “take ownership”, but maybe it’s because of this section in ldap_init_fd()


	/* Attach the passed socket as the LDAP's connection */
	conn = ldap_new_connection( ld, NULL, 1, 0, NULL, 0, 0 );
	if( conn == NULL ) {
		LDAP_MUTEX_UNLOCK( &ld->ld_conn_mutex );
		ldap_unbind_ext( ld, NULL, NULL );
		return( LDAP_NO_MEMORY );
	}
	if( url )
		conn->lconn_server = ldap_url_dup( ld->ld_options.ldo_defludp );
	ber_sockbuf_ctrl( conn->lconn_sb, LBER_SB_OPT_SET_FD, &fd );
	ld->ld_defconn = conn;
	++ld->ld_defconn->lconn_refcnt;	/* so it never gets closed/freed */

However this just means the connection won’t be reclaimed while LDAP object is in use, but that doesn’t stop it from being cleaned up on free.

If I’m following things correctly:
ldap.initialize(*args, fileno=fileno) will create a ldap object with an ldap connection from the connected socket fd we pass in.

But on ldap_unbind_ext() which ldap_destroy(), ldap_unbind(), dealloc(LDAPObject *self) etc, eventually call. ldap_unbind_ext() will call ldap_ld_free which if the ref count for the ldap object is < 2, it will perform

	/* free and unbind from all open connections */
	while ( ld->ld_conns != NULL ) {
		ldap_free_connection( ld, ld->ld_conns, /*force=*/1, /*unbind=*/close);
	}

Well ldap_free_connection() when force=1, will ignore the connection refcount and free the ldap connection anyway.

Specifically it frees/closes the fd by calling either of these functions

if ( lc->lconn_sb != ld->ld_sb ) {
			ber_sockbuf_free( lc->lconn_sb );
		} else {
			ber_int_sb_close( lc->lconn_sb );
		}

Where _sockbuf_free() also calls ber_int_sb_close()

And that function is

Sockbuf_IO_Desc		*p;

	assert( sb != NULL);
   
	p = sb->sb_iod;
	while ( p ) {
		if ( p->sbiod_io->sbi_close && p->sbiod_io->sbi_close( p ) < 0 ) {
			return -1;
		}
		p = p->sbiod_next;
	}
   
	sb->sb_fd = AC_SOCKET_INVALID;
   
	return 0;

Going way back to the Sockbuf_IO functions that were bound during the initalize discussed above.

Which means for all the supported protocols in python-ldap (TCP, UDP, IPC) at minimum close(fd) is called.

LDAPObject’s deconstructor will call ldap_unbind_ext() which as described above means it will always lead to closing the FD passed in to ldap.initalize(*args, fileno=fileno)

ldap_unbind_ext(3) is defined as

ldap_unbind_ext(3)
synchronously unbind from the LDAP server and close the
connection
So it seems like this is somewhat expected.

Why do the Docs believe libopenldap does not take ownership?

Steps to reproduce:
The simplest repro is

import ldap
import socket
# set openldap debugging levels
ldap.set_option(ldap.OPT_DEBUG_LEVEL, 255)
# monkey patch __del__ method to see when it is called
ldap.ldapobject.LDAPObject.__del__ = lambda self: print(f"calling __del__ of {self}")

def simple_repro():
    # Connect to an "LDAP server" (that doesn't exist)
    client, server = socket.socketpair(family=socket.AF_UNIX, type=socket.SOCK_STREAM)
    print(f"client={client.fileno()}, server={server.fileno()}")
    l = ldap.initialize('ldap://localhost:1389', trace_level=1, fileno=client.fileno())
    print(f"l's fileno={l.fileno()}")
    del l # force l's deconstruction
    server.close()
    # this will fail due to l's deconstruction resulting in close(client.fileno())
    client.close()

simple_repro()

Which outputs

client=3, server=4
ldap_url_parse_ext(ldap://localhost:1389)
ldap_create
ldap_url_parse_ext(ldap://localhost:1389)
ldap_new_connection 1 0 0
*** <ldap.ldapobject.SimpleLDAPObject object at 0x7fbcde13f3d0> ldap://localhost:1389 - SimpleLDAPObject.set_option
((17, 3), {})
*** <ldap.ldapobject.SimpleLDAPObject object at 0x7fbcde13f3d0> ldap://localhost:1389 - SimpleLDAPObject.get_option
((1,), {})
l's fileno=3
calling __del__ of <ldap.ldapobject.SimpleLDAPObject object at 0x7fbcde13f3d0>
ldap_free_connection 1 1
ldap_send_unbind
ldap_free_connection: actually freed
Traceback (most recent call last):
  File "examples/repro.py", line 19, in <module>
    simple_repro()
  File "examples/repro.py", line 17, in simple_repro
    client.close()
  File "/opt/python/python-3.11/lib64/python3.11/socket.py", line 503, in close
    self._real_close()
  File "/opt/python/python-3.11/lib64/python3.11/socket.py", line 497, in _real_close
    _ss.close(self)
OSError: [Errno 9] Bad file descriptor

If you run strace on it the relevant evidence is

socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, [3, 4]) = 0
getsockname(3, {sa_family=AF_UNIX}, [128 => 2]) = 0
getsockname(4, {sa_family=AF_UNIX}, [128 => 2]) = 0
write(1, "client=3, server=4\n", 19)    = 19
write(2, "ldap_url_parse_ext(ldap://localh"..., 42) = 42
write(2, "ldap_create\n", 12)           = 12
write(2, "ldap_url_parse_ext(ldap://localh"..., 42) = 42
write(2, "ldap_new_connection 1 0 0\n", 26) = 26
write(1, "*** <ldap.ldapobject.SimpleLDAPO"..., 130) = 130
write(1, "*** <ldap.ldapobject.SimpleLDAPO"..., 127) = 127
write(1, "l's fileno=3\n", 13)          = 13
write(1, "calling __del__ of <ldap.ldapobj"..., 79) = 79
write(2, "ldap_free_connection 1 1\n", 25) = 25
write(2, "ldap_send_unbind\n", 17)      = 17
write(3, "0\5\2\1\1B\0", 7)             = 7
shutdown(3, SHUT_RDWR)                  = 0
close(3)                                = 0
write(2, "ldap_free_connection: actually f"..., 37) = 37
close(4)                                = 0
close(3)                                = -1 EBADF (Bad file descriptor)

Operating system:
Linux
Python version:
Python 3.11.8
python-ldap version:
3.4.0
@mistotebe
Copy link
Contributor

Thanks for digging into this, I stumbled upon this in #544 but never clocked that we have documentation to the contrary.

There is no record of why the code suggests that ownership will not be claimed, libldap certainly suggests nothing either way (and as you figured, all builtin sockbuf implementations will close the fd passed in).

I would be OK with a documentation change + removal of the refcnt increment code, making it the caller's responsibility to ensure they don't touch/close the fd themselves.

Pinging @tiran as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ClydeByrdIII @mistotebe