From: gilles@poolp.org
Subject: Re: aldap_parse: infinit loop when connection closed
To: Omar Polo <op@omarpolo.com>, Philipp <philipp+openbsd@bureaucracy.de>
Cc: tech@openbsd.org
Date: Tue, 20 Feb 2024 22:32:23 +0100

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 ? :-)


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