From: Philipp <philipp+openbsd@bureaucracy.de>
Subject: Re: aldap_parse: infinit loop when connection closed
To: tech@openbsd.org
Cc: gilles@poolp.org, Omar Polo <op@omarpolo.com>
Date: Wed, 21 Feb 2024 10:25:35 +0100

[2024-02-20 22:32] gilles@poolp.org
> February 20, 2024 7:21 PM, "Omar Polo" <op@omarpolo.com> wrote:
>
> > Hello,
> > 
> > On 2024/02/19 14:10:37 +0100, Philipp <philipp+openbsd@bureaucracy.de> wrote:
> > 
> >> Hi
> >> 
> >> I noticed that aldap_parse() get stuck in an infinit loop when the fd is
> >> closed. The read loop only breaks when successfull parse a message or
> >> the read fails. But read() on a closed fd return 0 not -1. A patch is
> >> attached.
> >> 
> >> Philipp
> > 
> > I don't use ldap so can't properly test, but the proposed diff in itself
> > seems fine to me. We should handle the interrupted connection
> > gracefully. Maybe it could use a different error type since it's not
> > really a parser error, but I'm nitpicking.
> > 
> > so, fwiw, ok op@ :)
> > 
>
> that was my exact comment in private, it feels weird that the write path
> uses ALDAP_ERR_OPERATION_FAILED for i/o errors and that the read path is
> translating i/o errors as ALDAP_ERR_PARSER_ERROR making it impossible to
> determine if the a query failed because of i/o or data format.
>
> I think a better approach would be as below, but I don't have ldap and I
> don't know aldap enough to be confident it doesn't break something else.
>
> any aldap gurus around ? :-)

Setting a different error doesn't change anything. First of all because
no aldap.c consumer read err after a failed aldap_parse(). Secound the
handling of a parse error and a operational error is more or less the
same: Close the fd and optional try again. So the correct err value only
adds better logging ability.

Don't get me wrong: I would also say that the correct err value should
be set. I just wanted to point out that it doesn't break anything.

btw: ldapc_search() of usr.bin/ldap, client_search_idm() of
usr.sbin/ypldap, and search() of libexec/login_ldap don't handle
failures of aldap_parse() correct. Also libexec/login_ldap has some
memleaks caused by search() in combination with utoa(). Probably not the
only bugs in there. I just had a quick look.

Philipp

>
> Index: aldap.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ldap/aldap.c,v
> diff -u -p -r1.10 aldap.c
> --- aldap.c	31 Mar 2022 09:03:48 -0000	1.10
> +++ aldap.c	20 Feb 2024 21:28:13 -0000
> @@ -369,8 +369,8 @@ aldap_parse(struct aldap *ldap)
>  			} else
>  				ret = read(ldap->fd, rbuf, sizeof(rbuf));
>  
> -			if (ret == -1) {
> -				goto parsefail;
> +			if (ret <= 0) {
> +				goto opfail;
>  			}
>  
>  			evbuffer_add(ldap->buf, rbuf, ret);
> @@ -443,9 +443,16 @@ aldap_parse(struct aldap *ldap)
>  	}
>  
>  	return m;
> -parsefail:
> +
> + parsefail:
>  	evbuffer_drain(ldap->buf, EVBUFFER_LENGTH(ldap->buf));
>  	ldap->err = ALDAP_ERR_PARSER_ERROR;
> +	aldap_freemsg(m);
> +	return NULL;
> +
> + opfail:
> +	evbuffer_drain(ldap->buf, EVBUFFER_LENGTH(ldap->buf));
> +	ldap->err = ALDAP_ERR_OPERATION_FAILED;
>  	aldap_freemsg(m);
>  	return NULL;
>  }