From: Omar Polo <op@omarpolo.com>
Subject: Re: aldap_parse: infinit loop when connection closed
To: gilles@poolp.org
Cc: Philipp <philipp+openbsd@bureaucracy.de>, tech@openbsd.org
Date: Wed, 15 May 2024 11:36:45 +0200

On 2024/02/20 22:32:23 +0100, gilles@poolp.org wrote:
> 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 ? :-)

I don't have a ldap setup too (yet), but the diff is quite evident to
me.  Both read(2) and tls_read(3) can return 0 to signal EOF, and I
don't see the point in endlessly looping on reading from a closed
socket.

Philipp' point about the fact that the exact error is ignored is right,
but I'd still propend towards your diff since it still may help
debugging.

So, I'd go ahead with this.  Enough time has passed IMHO.  ok op@

p.s. there are also some further improvements to aldap.c that Philipp is
making to the copy in (OpenSMTPD) table-ldap which will make sense to
keep in sync here too eventually.

> 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;
>  }