Pleaes remove hard tabs and fix indentation.<br><br>Does this still pass all the tests?<br><br>Pretty obvious code duplication. Can't that be shared? What about:<br><br>DJabberd::Connection::add_ssl_disconnect_handler($conn, $ssl, $ctx);<br>
<br>When ready, would you mind submitting your next patch to <a href="http://codereview.appspot.com/">http://codereview.appspot.com/</a> ? I've added djabberd as a repo there.<br><br>Thanks!<br>Brad<br><br><div class="gmail_quote">
On Mon, Jun 16, 2008 at 8:24 AM, Jacob Burkhart <<a href="mailto:igotimac@gmail.com">igotimac@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div>Hi,<div><br></div><div>Please consider committing this patch to djabberd as a solution to a memory leak we are observing in SSL connections.</div><div><br></div><div>thanks,</div><div>Jacob</div><div><div></div><div class="Wj3C7c">
<div><br></div><div class="gmail_quote">
On Thu, Mar 6, 2008 at 3:26 PM, Jacob Burkhart <<a href="mailto:igotimac@gmail.com" target="_blank">igotimac@gmail.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Martin,<div><br></div><div>What if we did it via Connection add_disconnect_handler? (see attached)</div>
<div><br></div>
<div>Instead of having <a href="http://Connection.pm" target="_blank">Connection.pm</a> 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).</div>
<div><br></div><div>try it on for size...</div><div>Jacob</div><div><div></div><div><div><br></div><div><br><div class="gmail_quote">On Thu, Mar 6, 2008 at 1:18 PM, Martin Atkins <<a href="mailto:mart@degeneration.co.uk" target="_blank">mart@degeneration.co.uk</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div>Jacob Burkhart wrote:<br>
> I didn't realize it was possible to have multiple VHosts. Is it<br>
> possible to have multiple certs too?<br>
><br>
> <a href="http://code.sixapart.com/trac/djabberd/browser/trunk/djabberd.conf" target="_blank">http://code.sixapart.com/trac/djabberd/browser/trunk/djabberd.conf</a><br>
><br>
> The example config shows the certs being configred at the top level<br>
> (outside the context of the VHost), so I assumed it was not possible to<br>
> have more than one set of certs. I'll look into it more...<br>
><br>
<br>
</div>Actually, I think you're right. I misread the lines of code that<br>
reference the cert settings:<br>
<br>
Net::SSLeay::CTX_use_certificate_file(<br>
$ctx,<br>
$conn->vhost->server->ssl_cert_file,<br>
<br>
&Net::SSLeay::FILETYPE_PEM<br>
);<br>
<br>
I didn't notice the ->server in there.<br>
<br>
However, the same thing still applies: you could potentially have<br>
several servers running in the same process too. The stock djabberd<br>
script doesn't do this, but djabberd can be embedded in other stuff as<br>
well. I'm not sure what the best solution is, though. Having the SSL<br>
context in the server object would mean that the server core would<br>
depend on Net::SSLeay, which is a bit of a layering violation.<br>
<br>
It seems strange to me that the cert would be server-wide, since certs<br>
normally have the domain name in them so you'd need a different cert for<br>
each domain and thus each vhost. Maybe I'm just misunderstanding how all<br>
this works, though.<br>
<br>
Would you mind cooking up a patch that just calls CTX_free at an<br>
appropriate moment to fix the memory leak? I'll try to get hold of Brad<br>
(who wrote this SSL stuff) and see what he thinks the SSL context object<br>
is supposed to "belong" to.<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>
</blockquote></div><br>