multiprocessing.connection.Client deadlocks when trying to connect to a listener without a password

[MarcinKonowalczyk]

Bug report

Bug description:

We set up a multiprocessing.connection.Listener with no authkey. When we try to connect to it with a Client with an authkey, the Client deadlocks. Here is a working example:

import socket
import threading
import time
from multiprocessing.connection import Client, Listener


def _test(*, listener: str | None, client: str | None) -> tuple[Exception | None, Exception | None, list]:
    _autkey_listener = listener.encode() if listener else None
    _authkey_client = client.encode() if client else None

    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
        s.bind(("localhost", 0))
        address = s.getsockname()

    thread_exc = None
    thread_got = []
    client_exc = None

    def target() -> None:
        try:
            with Listener(address, authkey=_autkey_listener) as l:
                c = l.accept()
                while True:
                    value = c.recv()
                    if not value:
                        break
                    thread_got.append(value)
        except Exception as e:
            nonlocal thread_exc
            thread_exc = e

    t = threading.Thread(target=target)
    t.start()

    try:
        with Client(address, authkey=_authkey_client) as c:
            c.send("hello")
            time.sleep(0.1)
            c.send(None)

        t.join()
    except Exception as e:
        client_exc = e

    return thread_exc, client_exc, thread_got


if __name__ == "__main__":
    print(" no passwords ".center(40, "-"))
    print(_test(listener=None, client=None))

    print(" two matching passwords ".center(40, "-"))
    print(_test(listener="password", client="password"))

    print(" client has wrong password ".center(40, "-"))
    print(_test(listener="password", client="wrong"))

    print(" client has no password ".center(40, "-"))
    print(_test(listener="password", client=None))

    # This is the case which deadlocks
    print(" listener has no password ".center(40, "-"))
    print(_test(listener=None, client="password"))

which outputs:

------------- no passwords -------------
(None, None, ['hello'])
-------- two matching passwords --------
(None, None, ['hello'])
------ client has wrong password -------
(AuthenticationError('digest received was wrong'), AuthenticationError('digest sent was rejected'), [])
-------- client has no password --------
(AuthenticationError("expected 'md5' of length 16 got 20"), None, [])
------- listener has no password -------

and then stops.

Upon some investigation, the following bit of code from Client is to blame:

  if authkey is not None:
      answer_challenge(c, authkey)
      deliver_challenge(c, authkey)

since the listener is not configured to deliver the challenge to the connections. I think this would be resolved with an optional timeout kwarg in answer_challenge. If this suggestion is accepted I would be very happy to work on it 👍

CPython versions tested on:

3.9, 3.10, 3.11, 3.12

Operating systems tested on:

macOS, Windows

[picnixz]

I'm not sure how to categorize it. I feel it's a bug because we don't want the client to possibly be deadlocked. On the other hand, I'd say it's a feature because when setting the connection, you'd expect both participants to at least agree on the kind of protocol (namely, at least agree on whether an authentication is needed or not).

I am personally in favor of adding a timeout keyword argument to def Client(...) which would times out if the answer/delivery takes too much time:

def Client(..., *, timeout=None): ...
def answer_challenge(client, authkey, *, timeout=None): ...
def deliver_challenge(client, authkey, *, timeout=None): ...

[MarcinKonowalczyk]

i'd also then add a def accept(self, *, timeout=None): ... on Listener to allow protecting the server from a malicious client who does not reply to deliver_challenge

[MarcinKonowalczyk]

worth adding this to the Multiprocessing issues project?

also; @picnixz, should i wait a bit for anyone else to have a look (@vstinner ?)? can i attempt adding the timeout and make a PR?

[picnixz]

I'd prefer waiting for a core dev's opinion on that matter. Personally I think having a timeout makes sense but someone with more insight on this part of the library would be good.

[MarcinKonowalczyk]

@vstinner could you have a look at this ^ ? alternatively, could you tell me who's the right core dev to poke about it?

to clarify: i'd love to work on this issue, but following picnixz advice i wanted to get a blessing from a core dev.

[vstinner]

alternatively, could you tell me who's the right core dev to poke about it?

I don't know who can help on this issue. Maybe @pitrou?