Hey Brad, about the code duplication, isn't a really big part of StartTLS and OldSSLClientIn essentially duplicated code? I saw this and assumed there must be some good reason for it, so I did the same with my additions... should I try to tackle removing this duplication too?
Tests appear to be passing... is there a test that specifically tests SSL? Is there a faster way to run tests other than: perl Makefile.PL && make && make test ? I'll upload a new diff to codereview tomorrow Jacob On Mon, Jun 16, 2008 at 11:55 PM, Brad Fitzpatrick <[EMAIL PROTECTED]> wrote: > Pleaes remove hard tabs and fix indentation. > > Does this still pass all the tests? > > Pretty obvious code duplication. Can't that be shared? What about: > > DJabberd::Connection::add_ssl_disconnect_handler($conn, $ssl, $ctx); > > When ready, would you mind submitting your next patch to > http://codereview.appspot.com/ ? I've added djabberd as a repo there. > > Thanks! > Brad > > > On Mon, Jun 16, 2008 at 8:24 AM, Jacob Burkhart <[EMAIL PROTECTED]> > wrote: > >> Hi, >> Please consider committing this patch to djabberd as a solution to a >> memory leak we are observing in SSL connections. >> >> thanks, >> Jacob >> >> On Thu, Mar 6, 2008 at 3:26 PM, Jacob Burkhart <[EMAIL PROTECTED]> >> wrote: >> >>> Martin, >>> What if we did it via Connection add_disconnect_handler? (see attached) >>> >>> Instead of having Connection.pm check for a $self->{ssl} and do the >>> appropriate free on close, we could have StartTLS (and OldSSLClientIn) add a >>> callback to the close. (And so the CTX object is referenced in that >>> callback). >>> >>> try it on for size... >>> Jacob >>> >>> >>> On Thu, Mar 6, 2008 at 1:18 PM, Martin Atkins <[EMAIL PROTECTED]> >>> wrote: >>> >>>> Jacob Burkhart wrote: >>>> > I didn't realize it was possible to have multiple VHosts. Is it >>>> > possible to have multiple certs too? >>>> > >>>> > http://code.sixapart.com/trac/djabberd/browser/trunk/djabberd.conf >>>> > >>>> > The example config shows the certs being configred at the top level >>>> > (outside the context of the VHost), so I assumed it was not possible >>>> to >>>> > have more than one set of certs. I'll look into it more... >>>> > >>>> >>>> Actually, I think you're right. I misread the lines of code that >>>> reference the cert settings: >>>> >>>> Net::SSLeay::CTX_use_certificate_file( >>>> $ctx, >>>> $conn->vhost->server->ssl_cert_file, >>>> >>>> &Net::SSLeay::FILETYPE_PEM >>>> ); >>>> >>>> I didn't notice the ->server in there. >>>> >>>> However, the same thing still applies: you could potentially have >>>> several servers running in the same process too. The stock djabberd >>>> script doesn't do this, but djabberd can be embedded in other stuff as >>>> well. I'm not sure what the best solution is, though. Having the SSL >>>> context in the server object would mean that the server core would >>>> depend on Net::SSLeay, which is a bit of a layering violation. >>>> >>>> It seems strange to me that the cert would be server-wide, since certs >>>> normally have the domain name in them so you'd need a different cert for >>>> each domain and thus each vhost. Maybe I'm just misunderstanding how all >>>> this works, though. >>>> >>>> Would you mind cooking up a patch that just calls CTX_free at an >>>> appropriate moment to fix the memory leak? I'll try to get hold of Brad >>>> (who wrote this SSL stuff) and see what he thinks the SSL context object >>>> is supposed to "belong" to. >>>> >>>> >>> >> >
