Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

2017-12-17 6:35 GMT+03:00 Jonathan Matthew :
> On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
>> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov :
>> >> 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 -  1.37
> +++ aldap.c 17 Dec 2017 03:19:02 -
> @@ -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(>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

Re: ypldap patch 5: fix aldap_close

[Jonathan Matthew]

On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov :
> >> 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 -  1.37
+++ aldap.c 17 Dec 2017 03:19:02 -
@@ -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(>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);
 }
 

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

2017-12-06 19:12 GMT+03:00 Vadim Zhukov :
>> 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.

> Index: aldap.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 aldap.c
> --- aldap.c 30 May 2017 09:33:31 -  1.37
> +++ aldap.c 6 Dec 2017 13:11:59 -
> @@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
> return BER_TYPE_OCTETSTRING;
>  }
>
> -int
> +void
>  aldap_close(struct aldap *al)
>  {
> if (al->fd != -1)
> -   if (close(al->ber.fd) == -1)
> -   return (-1);
> -
> +   close(al->fd);
> ber_free(>ber);
> evbuffer_free(al->buf);
> free(al);
> -
> -   return (0);
>  }
>
>  struct aldap *
> Index: aldap.h
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 aldap.h
> --- aldap.h 30 May 2017 09:33:31 -  1.10
> +++ aldap.h 6 Dec 2017 13:11:59 -
> @@ -206,7 +206,7 @@ enum subfilter {
>  struct aldap   *aldap_init(int);
>  int aldap_tls(struct aldap *, struct tls_config *,
> const char *);
> -int aldap_close(struct aldap *);
> +voidaldap_close(struct aldap *);
>  struct aldap_message   *aldap_parse(struct aldap *);
>  voidaldap_freemsg(struct aldap_message *);
>

--
  WBR,
  Vadim Zhukov

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

> 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.

Okay?

--
WBR,
  Vadim Zhukov


Index: aldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.37
diff -u -p -r1.37 aldap.c
--- aldap.c 30 May 2017 09:33:31 -  1.37
+++ aldap.c 6 Dec 2017 13:11:59 -
@@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
return BER_TYPE_OCTETSTRING;
 }
 
-int
+void
 aldap_close(struct aldap *al)
 {
if (al->fd != -1)
-   if (close(al->ber.fd) == -1)
-   return (-1);
-
+   close(al->fd);
ber_free(>ber);
evbuffer_free(al->buf);
free(al);
-
-   return (0);
 }
 
 struct aldap *
Index: aldap.h
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
retrieving revision 1.10
diff -u -p -r1.10 aldap.h
--- aldap.h 30 May 2017 09:33:31 -  1.10
+++ aldap.h 6 Dec 2017 13:11:59 -
@@ -206,7 +206,7 @@ enum subfilter {
 struct aldap   *aldap_init(int);
 int aldap_tls(struct aldap *, struct tls_config *,
const char *);
-int aldap_close(struct aldap *);
+voidaldap_close(struct aldap *);
 struct aldap_message   *aldap_parse(struct aldap *);
 voidaldap_freemsg(struct aldap_message *);
 

ypldap patch 5: fix aldap_close

[Vadim Zhukov]

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.

Okay?

--
WBR,
  Vadim Zhukov


Index: aldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.37
diff -u -p -r1.37 aldap.c
--- aldap.c 30 May 2017 09:33:31 -  1.37
+++ aldap.c 6 Dec 2017 13:11:59 -
@@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
return BER_TYPE_OCTETSTRING;
 }
 
-int
+void
 aldap_close(struct aldap *al)
 {
if (al->fd != -1)
-   if (close(al->ber.fd) == -1)
-   return (-1);
-
+   close(al->ber.fd);
ber_free(>ber);
evbuffer_free(al->buf);
free(al);
-
-   return (0);
 }
 
 struct aldap *
Index: aldap.h
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
retrieving revision 1.10
diff -u -p -r1.10 aldap.h
--- aldap.h 30 May 2017 09:33:31 -  1.10
+++ aldap.h 6 Dec 2017 13:11:59 -
@@ -206,7 +206,7 @@ enum subfilter {
 struct aldap   *aldap_init(int);
 int aldap_tls(struct aldap *, struct tls_config *,
const char *);
-int aldap_close(struct aldap *);
+voidaldap_close(struct aldap *);
 struct aldap_message   *aldap_parse(struct aldap *);
 voidaldap_freemsg(struct aldap_message *);