[PATCH] minor leak in StartTLS

[Brad Fitzpatrick]

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 <igotimac at gmail.com> 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 <igotimac at gmail.com> 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 <mart at degeneration.co.uk>
>> 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.
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.danga.com/pipermail/djabberd/attachments/20080616/e87b47f9/attachment.html