jstedfast / MailKit

A cross-platform .NET library for IMAP, POP3, and SMTP.
http://www.mimekit.net
MIT License
6.22k stars 824 forks source link

Memory leak in Crypto32 APi Class SafeCertContextHandle due to Mailkit handling of SSL Certs #1613

Closed salusalpha closed 1 year ago

salusalpha commented 1 year ago

Describe the bug Memory leak in Crypto32 APi Class SafeCertContextHandle due to Mailkit handling of SSL Certs

Platform (please complete the following information):

Exception out of memory

To Reproduce I am connecting to an Imap server on port 143 with username and password

Expected behavior no memory leak

Code Snippets if i connect like

var client = new ImapClient(); await client.ConnectAsync(ImapServer, 143, false)

i get after approx 12 hours 2 GB of unmanaged memory due to ssl handling in crypto32. the certificate is a LetsEncrypt certificate

if i connect like this the memory leak is not happening. await client.ConnectAsync(ImapServer, 143, SecureSocketOptions.None)

jstedfast commented 1 year ago

client.Connect(hostName, port, false); will use STARTTLS if it is available. In other words, it's equivalent of client.Connect(hostName, port, SecureSocketOptions.StartTlsWhenAvailable); - it's not equivalent to SecureSocketOptions.None.

That said, it shouldn't have a memory leak, but I'm not sure how I could fix that leak. Could you elaborate on what MailKit is doing wrong, exactly?

salusalpha commented 1 year ago

yes correct. the memory leak is also there if client.Connect(hostName, port, SecureSocketOptions.Auto) is used.

When writing code like below

using (var client = new ImapClient()) {
await client.ConnectAsync(ImapServer, 143, SecureSocketOptions.Auto);

  await client.AuthenticateAsync(username, password);     

  await client.DisconnectAsync(true);

}

I would expect that resources are freed including the certificate context. It seems like that MailKit is not releasing the certificate context after Authentication / after Disconnect. My app is running in 5 min cycles checking IMAP mailboxes for new mails. Every 5 min cycle it leaks like 20mb. The pic is 2 runs for 10 mins and already 40 mb leaked. It's essential to ensure that the certificate context is released correctly to prevent memory leaks. Maybe CertFreeCertificateContext can help.
Capture

jstedfast commented 1 year ago

Where would MailKit release this certificate context? MailKit never even has access to any CertificateContext so this seems like an impossible thing for MailKit to do.

All MailKit does is call SslStream.AuthenticateAsClient(hostName, null, sslProtocols, true) - MailKit doesn't ever even have access to the certificates.

jstedfast commented 1 year ago

I'm pretty confident that the problem is not in MailKit and is a bug in the .NET libraries.

jstedfast commented 1 year ago

Duplicate of https://github.com/jstedfast/MailKit/issues/1105

jstedfast commented 1 year ago

You may be interested in this comment: https://github.com/jstedfast/MailKit/issues/1105#issuecomment-896185615

salusalpha commented 1 year ago

Hi. it is not a duplicate if you ask me of #1105. As a matter of fact i was reading #1105 before and followed actually also this client.CheckCertificateRevocation = false; which made zero difference. Also i am on windows not linux.

I asked chatgpt what it says if it is a bug in Mailkit or .net

Answer:

If the certificate context is not released and there is a memory leak, it is most likely a bug in the MailKit library rather than the .NET Framework itself. The MailKit library is responsible for managing resources related to email communication, including certificate handling when connecting to secure servers like IMAP servers over SSL/TLS.

salusalpha commented 1 year ago

Can you please confirm that MaiIkit properly disposes the SslStream instance when you are done using it. Disposing the SslStream will ensure that any associated resources, including the certificate context, are released properly.

some research I did:

If you use the using construct with SslStream, as shown in the following code:

using (var sslStream = new SslStream(stream, false)) { // read from stream here... }

The using statement will automatically dispose of the SslStream instance when the code block is exited. It calls Dispose() on the SslStream, which, in turn, closes the underlying stream (the inner stream), as you passed false as the second parameter. So, when using the using construct, you don't need to manually call Dispose() on SslStream or the inner stream.

If you want to keep the SslStream for later use and don't want it to close the underlying stream when it's disposed, you can set the second parameter to true like this:

var sslStream = new SslStream(stream, true); // Use sslStream for communication... // Remember to dispose of sslStream manually when you are done with it. sslStream.Dispose();

In this case, when you call Dispose() on the sslStream, it won't close the underlying stream. You'll need to manually dispose of the sslStream using Dispose() when you are done with it.

Additionally, if the underlying NetworkStream is created with ownsSocket set to true, like this:

var networkStream = new NetworkStream(socket, true); var sslStream = new SslStream(networkStream, true); // Use sslStream for communication... // Dispose of sslStream will also close the underlying networkStream and its socket. sslStream.Dispose();

Then, disposing the sslStream will also close the underlying NetworkStream and its socket. So, in this case, you don't need to manually dispose of the NetworkStream.

In summary, using the using construct is a good practice when working with SslStream, as it automatically takes care of disposing the instance and its underlying stream when you're done with it. If you need to keep the SslStream for later use, you can set the second parameter to true in the constructor, and you'll need to dispose of it manually when you're done. The same logic applies to the NetworkStream created with ownsSocket set to true.

jstedfast commented 1 year ago

Yes, it properly disposes SslStream when it is done with it.

If you can find me a case where it doesn't, I'd be more than happy to fix that. I thoroughly went through every scenario when I was investigating that other issue and it disposed the SslStream in every conceivable scenario which was all verified by logging that I had added at the time.

jstedfast commented 1 year ago

FWIW, disposing the imapClient disposes the SslStream.

salusalpha commented 1 year ago

Hi Jeffrey.

I did more tests. I upgraded to net5 and found the same behaviour. a memory leak in crypot32 due to certificate bytes piling up and not being released. It is hard to believe that every .net version has the issue.

I then tried to play a bit with your mailkit code trying to make some Dispose here or there and even tried to play with CertFreeCertificateContext but your code is complex so I did not succeed.

I also did a short program

using System;
using MailKit.Net.Imap;
using MailKit.Security;

static void Main()
{
    // Simulate a loop
    while (true)
    {
        try
        {
            using (var client = new ImapClient())
            {
                //var client = new ImapClient();

                client.ServerCertificateValidationCallback = (s, c, h, e) => true;
                client.CheckCertificateRevocation = false;

                // Establish a connection with the IMAP server
                client.Connect("myimap", 143, SecureSocketOptions.Auto);

                // Authenticate the client
                client.Authenticate("xxx@yyy.com", "123456789");

                if (client.IsAuthenticated) Console.WriteLine($"{DateTime.Now} Auth ok");

                // Do some IMAP operations, but for demonstration purposes, we'll just sleep
                System.Threading.Thread.Sleep(1000);

                // The using block above will automatically dispose of the client
                // and close the underlying TcpClient connection
                client.Disconnect(true);
                if (!client.IsConnected) Console.WriteLine($"{DateTime.Now} Disconnected");
                //client.Dispose();
            }
        }
        catch (Exception ex)
        {
            // Handle exceptions, but for this demonstration, we'll just ignore them
        }
    }
} 

I did get the leak in net 4.7.2 with above code. You can nicely see in VS under "Memory Usage". It is piling up certificate bytes.

Capture

When i then upgraded to net5 i felt it was fixed but then with the full code was exactly the same. So something is Microsoft expecting to be done differently as I am still arguing it is not a bug in .Net framework. I also cannot say if there is something special with LetsEncrypt certificates or our IMAP server. If you need our IMAP server for testing I am happy to provide.

Sorry that i can not add more insight but it is very sad that such a powerful library like mailkit is rendered useless due to this memory leak.

jstedfast commented 1 year ago

Add logging to ImapClient.cs's ValidateRemoteCertificate method.

Does that get called?

It's literally the only way MailKit could ever have anything to do with a server certificate.

jstedfast commented 1 year ago

If you want to verify that the SslStream is being disposed, that happens in ImapEngine.cs's Disconnect() method.

jstedfast commented 1 year ago

I did get the leak in net 4.7.2 with above code. You can nicely see in VS under "Memory Usage". It is piling up certificate bytes.

If that's a "leak", then it is in PinnableBufferCache which is not MailKit code and you'll notice that there are no MailKit classes in the expanded tree that shows what still has references to it.

salusalpha commented 1 year ago

Hi Jeffrey.

Please run the code provided and please see. I am sure your expertise level is above mine. I also see you have a microsoft email address so guess it is an ease for you to talk to MS to find a solution.

thanks and two thumbs up for the effort.

jstedfast commented 1 year ago

My point it that I can't reproduce any memory leak and the SslStream is always disposed, so there's no possible way for MailKit's ImapClient to leak anything related to the SslStream.

jstedfast commented 1 year ago

BTW, your screenshot is not a leak. It's simply memory that hasn't been collected by the GC yet. The GC is not instant.

salusalpha commented 1 year ago

let the code run. in task manager select column "Commit Size" to watch and let it grow till eg 2GB and the code catches out of memory exception

jstedfast commented 1 year ago

I guarantee that this leaks for you.

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    internal class Program
    {
        const string HostName = "imap.server.com";
        const int Port = 993;

        static void Main(string[] args)
        {
            while (true)
            {
                Console.WriteLine("Connecting...");
                ConnectAsync().GetAwaiter().GetResult();
                Thread.Sleep(1000);
                Console.WriteLine("Disconnected.");
            }
        }

        static async Task ConnectAsync()
        {
            using (var client = new TcpClient())
            {
                await client.ConnectAsync(HostName, Port);
                using (var sslStream = new SslStream(client.GetStream(), true, ValidateRemoteCertificate))
                {
                    await sslStream.AuthenticateAsClientAsync(HostName, clientCertificates: null, enabledSslProtocols: SslProtocols.Tls12 | SslProtocols.Tls13, checkCertificateRevocation: true);
                }
            }
        }

        static bool ValidateRemoteCertificate(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
        {
            Console.WriteLine("Validating remote certificate");
            return true;
        }
    }
}
jstedfast commented 1 year ago

Replace imap.server.com with your IMAP server.

jstedfast commented 1 year ago

As you saw in issue #1105, I could not reproduce that memory leak either which was exactly the same situation you are in where the SSL certificates were being "leaked".

The theory at the time was that it was only the Linux implementation of SslStream because everyone experiencing the issue was on Linux. But obviously that was a red herring because you are also seeing the very same leak on Windows.

The other theory was that this was dependent on the server's SSL certificate and/or certificate chain.

If you give me the URL for your IMAP server, I can test the simple TcpClient testcase I wrote to see if that leaks for me, but I am 100% positive that the leak is not in MailKit.

salusalpha commented 1 year ago

sure can give the URL. can i send it to your microsoft email ?

jstedfast commented 1 year ago

Yea, you can send it to my @microsoft.com address.

jstedfast commented 1 year ago

The following program has grown from ~15MB to 21MB in the span of a few minutes (with your server address being used):

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    internal class Program
    {
        static readonly byte[] StartTlsCommand = Encoding.ASCII.GetBytes("A01 STARTTLS\r\n");
        static readonly byte[] LogoutCommand = Encoding.ASCII.GetBytes("A02 LOGOUT\r\n");
        const string HostName = "mail.server.com";
        const int Port = 143;

        static void Main(string[] args)
        {
            byte[] buffer = new byte[1024];

            while (true)
            {
                Console.WriteLine("Connecting...");
                ConnectAsync(buffer).GetAwaiter().GetResult();
                Thread.Sleep(1000);
                Console.WriteLine("Disconnected.");
            }
        }

        static async Task ConnectAsync(byte[] buffer)
        {
            using (var client = new TcpClient())
            {
                await client.ConnectAsync(HostName, Port);

                var stream = client.GetStream();
                string response;
                int nread;

                // read the IMAP greeting ("* OK IMAP4rev1\r\n")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                response = Encoding.ASCII.GetString(buffer, 0, nread);

                // send the STARTTLS command
                await stream.WriteAsync(StartTlsCommand, 0, StartTlsCommand.Length);

                // read the STARTTLS response (something like "A01 OK STARTTLS successful")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                response = Encoding.ASCII.GetString(buffer, 0, nread);

                // negotiate SSL
                using (var sslStream = new SslStream(stream, true, ValidateRemoteCertificate))
                {
                    await sslStream.AuthenticateAsClientAsync(HostName, clientCertificates: null, enabledSslProtocols: SslProtocols.Tls12 | SslProtocols.Tls13, checkCertificateRevocation: true);

                    // send the LOGOUT command
                    await sslStream.WriteAsync(LogoutCommand, 0, LogoutCommand.Length);

                    // read the LOGOUT response (something like "* BYE\r\nA02 OK LOGOUT successful\r\n")
                    nread = await sslStream.ReadAsync(buffer, 0, buffer.Length);
                    response = Encoding.ASCII.GetString(buffer, 0, nread);
                }
            }
        }

        static bool ValidateRemoteCertificate(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
        {
            Console.WriteLine("Validating remote certificate");
            return true;
        }
    }
}
jstedfast commented 1 year ago

Your MailKit test program is climbing in memory usage at roughly the same rate.

jstedfast commented 1 year ago

I'll leave your MailKit test program run for the rest of the day to see where it ends up but so far I've watched the GC kick in twice and knock the memory usage back down to ~19MB once it got to roughly 22-24 MB.

jstedfast commented 1 year ago

45 minutes and still only 20 MB

salusalpha commented 1 year ago

image

please make sure you look at Commit Size in task manager

jstedfast commented 1 year ago

https://github.com/jstedfast/MailKit/assets/338984/2e93ab0b-f528-4a5b-b27d-0499ef418735

This is after 67+ minutes of running.

jstedfast commented 1 year ago

Notice how the Commit Size goes down every once in a while. That's probably because the GC kicked in.

It's just not going up (or at least not very quickly).

jstedfast commented 1 year ago

Have you tried reproducing the memory leak issue with the following test program?

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    internal class Program
    {
        static readonly byte[] StartTlsCommand = Encoding.ASCII.GetBytes("A01 STARTTLS\r\n");
        static readonly byte[] LogoutCommand = Encoding.ASCII.GetBytes("A02 LOGOUT\r\n");
        const string HostName = "mail.server.com";
        const int Port = 143;

        static void Main(string[] args)
        {
            byte[] buffer = new byte[1024];

            while (true)
            {
                Console.WriteLine("Connecting...");
                ConnectAsync(buffer).GetAwaiter().GetResult();
                Thread.Sleep(1000);
                Console.WriteLine("Disconnected.");
            }
        }

        static async Task ConnectAsync(byte[] buffer)
        {
            using (var client = new TcpClient())
            {
                await client.ConnectAsync(HostName, Port);

                var stream = client.GetStream();
                //string response;
                int nread;

                // read the IMAP greeting ("* OK IMAP4rev1\r\n")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                //response = Encoding.ASCII.GetString(buffer, 0, nread);

                // send the STARTTLS command
                await stream.WriteAsync(StartTlsCommand, 0, StartTlsCommand.Length);

                // read the STARTTLS response (something like "A01 OK STARTTLS successful")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                //response = Encoding.ASCII.GetString(buffer, 0, nread);

                // negotiate SSL
                using (var sslStream = new SslStream(stream, true, ValidateRemoteCertificate))
                {
                    await sslStream.AuthenticateAsClientAsync(HostName, clientCertificates: null, enabledSslProtocols: SslProtocols.Tls12 | SslProtocols.Tls13, checkCertificateRevocation: true);

                    // send the LOGOUT command
                    await sslStream.WriteAsync(LogoutCommand, 0, LogoutCommand.Length);

                    // read the LOGOUT response (something like "* BYE\r\nA02 OK LOGOUT successful\r\n")
                    nread = await sslStream.ReadAsync(buffer, 0, buffer.Length);
                    //response = Encoding.ASCII.GetString(buffer, 0, nread);
                }
            }
        }

        static bool ValidateRemoteCertificate(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
        {
            Console.WriteLine("Validating remote certificate");
            return true;
        }
    }
}
salusalpha commented 1 year ago

it is late here..i will do it tomorrow

jstedfast commented 1 year ago

Your MailKit-based test program is still hovering at ~21 MB after 268 minutes.

salusalpha commented 1 year ago

because is only one email account add 10 accounts and boom it goes

jstedfast commented 1 year ago

because is only one email account add 10 accounts and boom it goes

That sounds like there's a bug in either SslStream or in a cache that SslStream uses.

jstedfast commented 1 year ago

I left the test app (that used MailKit) running all night and the Commit Size is still only ~21 MB.

Based on your suggestion that the problem happens when 10 accounts are used, I modified the test program to create 10 threads all connecting/disconnecting to the mail server via SSL and it seemed to level off at about ~25 MB Commit Size. Every time it got to 25 MB, the GC seemed to kick in and it'd drop to ~24 MB and then climb back to 25, then drop to 24.

jstedfast commented 1 year ago

Okay, so I was testing this with net6.0 before. If I change the TargetFrameworks value in the .csproj for either of the 2 (multithreaded) test programs, I do see the program grow and grow and grow.

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    internal class Program
    {
        static readonly byte[] StartTlsCommand = Encoding.ASCII.GetBytes("A01 STARTTLS\r\n");
        static readonly byte[] LogoutCommand = Encoding.ASCII.GetBytes("A02 LOGOUT\r\n");
        const string HostName = "mail.server.com";
        const int Port = 143;

        static void Main(string[] args)
        {
            var threads = new Thread[10];

            for (int i = 0; i < threads.Length; i++)
            {
                threads[i] = new Thread(ThreadMain)
                {
                    Name = $"Thread {i}",
                    IsBackground = true
                };

                threads[i].Start();
            }

            for (int i = 0; i < threads.Length; i++)
                threads[i].Join();
        }

        static void ThreadMain()
        {
            byte[] buffer = new byte[1024];

            while (true)
            {
                Console.WriteLine($"{Thread.CurrentThread.Name}: Connecting...");
                ConnectAsync(buffer).GetAwaiter().GetResult();
                Thread.Sleep(1000);
                Console.WriteLine($"{Thread.CurrentThread.Name}: Disconnected.");
            }
        }

        static async Task ConnectAsync(byte[] buffer)
        {
            using (var client = new TcpClient())
            {
                await client.ConnectAsync(HostName, Port);

                var stream = client.GetStream();
                //string response;
                int nread;

                // read the IMAP greeting ("* OK IMAP4rev1\r\n")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                //response = Encoding.ASCII.GetString(buffer, 0, nread);

                // send the STARTTLS command
                await stream.WriteAsync(StartTlsCommand, 0, StartTlsCommand.Length);

                // read the STARTTLS response (something like "A01 OK STARTTLS successful")
                nread = await stream.ReadAsync(buffer, 0, buffer.Length);
                //response = Encoding.ASCII.GetString(buffer, 0, nread);

                // negotiate SSL
                using (var sslStream = new SslStream(stream, true, ValidateRemoteCertificate))
                {
                    await sslStream.AuthenticateAsClientAsync(HostName, clientCertificates: null, enabledSslProtocols: SslProtocols.Tls12, checkCertificateRevocation: true);

                    // send the LOGOUT command
                    await sslStream.WriteAsync(LogoutCommand, 0, LogoutCommand.Length);

                    // read the LOGOUT response (something like "* BYE\r\nA02 OK LOGOUT successful\r\n")
                    nread = await sslStream.ReadAsync(buffer, 0, buffer.Length);
                    //response = Encoding.ASCII.GetString(buffer, 0, nread);
                }
            }
        }

        static bool ValidateRemoteCertificate(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
        {
            Console.WriteLine($"{Thread.CurrentThread.Name}: Validating remote certificate");
            return true;
        }
    }
}

I'm currently running the above app with the TargetFramework set to net472 and it is already up to 47 MB and continuing to grow and has grown by ~15 MB since I started writing this comment.

jstedfast commented 1 year ago

...And now the GC kicked in and knocked it down to 42 MB. I'll leave it running for a bit to see if it levels out somewhere in the 42-47 MB range or if it grows past that.

jstedfast commented 1 year ago

Seems to be leveling off between 44-50 MB range after about ~15 minutes.

jstedfast commented 1 year ago

23 minutes and still only ~44 MB

jstedfast commented 1 year ago

I guess the solution for you is to upgrade to net6.0 :-)

BTW, if you aren't already aware, net5.0 is no longer supported.

salusalpha commented 1 year ago

hi jeffrey. Do you still need the test email/ server. What is the conclusion?

thanks

jstedfast commented 1 year ago

The conclusion is that it's not a bug in MailKit so I stopped trying to investigate further.

I was not able to reproduce the issue on net6.0 or net472.

Net6.0 never used more than ~21 MB and net472 stayed between 44 MB and 50 MB even after running for 12+ hours.