2017-12-17 6:35 GMT+03:00 Jonathan Matthew <jonat...@d14n.org>:
> On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
>> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
>> >> 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);
> }
Thank you for answering.
The diff reads correct to me, okay zhuk@.
--
WBR,
Vadim Zhukov
