Re: ypldap patch 5: fix aldap_close
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(&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
Re: ypldap patch 5: fix aldap_close
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(&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); }
Re: ypldap patch 5: fix aldap_close
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(&al->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
> 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(&al->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
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(&al->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 *);