On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov <[email protected]>:
> >> The aldap_close() return value is never checked, and I do not see
> >> any good reason to do that. Also, in case close(2) fails, it'll miss
> >> freeing other data.
> >
> > I'm blind. :-\
> >
> > The problem I was looking for was right here: the aldap_close() closes
> > the wrong file descriptor. So here is a better patch that solves
> > actual leak. I ever treat this as a candidate for -STABLE, since
> > when ypldap get stuck, you could be locked out of system.
>
> Sorry for noise, I'm just trying to make this patch go in. I think it
> should because it fixes a real issue seen in the wild (if an isolated
> AD-enabled LAN could be called "wild"). Well, actually it fixes two
> issues, but I found zero code paths for making close() fail in current
> code.
>
> The patched version happily runs for more than a week on (otherwise)
> 6.2-STABLE.
Your diff is correct, but only for the non-tls case. I missed cleaning
up the tls context when I added tls support, and we need to keep the fd
around so we can close it, since tls_close doesn't close file descriptors
that libtls didn't open.
ok?
Index: aldap.c
===================================================================
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.37
diff -u -p -u -p -r1.37 aldap.c
--- aldap.c 30 May 2017 09:33:31 -0000 1.37
+++ aldap.c 17 Dec 2017 03:19:02 -0000
@@ -70,10 +70,11 @@ aldap_application(struct ber_element *el
int
aldap_close(struct aldap *al)
{
- if (al->fd != -1)
- if (close(al->ber.fd) == -1)
- return (-1);
-
+ if (al->tls != NULL) {
+ tls_close(al->tls);
+ tls_free(al->tls);
+ }
+ close(al->fd);
ber_free(&al->ber);
evbuffer_free(al->buf);
free(al);
@@ -120,7 +121,6 @@ aldap_tls(struct aldap *ldap, struct tls
return (-1);
}
- ldap->fd = -1;
return (0);
}
