search

jstedfast / MailKit

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

ImapClient.ConnectAsync throws NullReferenceException with OTEL enabled #1765

Open georg-jung opened 2 days ago

georg-jung commented 2 days ago

Describe the bug

ImapClient.ConnectAsync throws NullReferenceException if it's activity source MailKit.Net.ImapClient is actually used with OTEL.

ImapClient.ConnectAsync calls engine.StartNetworkOperation, which in turn uses engine's Uri property before it is set. It would be set some LoC later: https://github.com/jstedfast/MailKit/blob/c39a209f6f196447e069306a13821f3b10b5ab7b/MailKit/Net/Imap/AsyncImapClient.cs#L830-L835

The NetworkOperation ctor tries to access Uri's properties, which fails because the reference is null: https://github.com/jstedfast/MailKit/blob/c39a209f6f196447e069306a13821f3b10b5ab7b/MailKit/Net/NetworkOperation.cs#L66-L69

I think this just happens if telemetry is enabled and .NET 6+ is used (I didn't test this). If telemetry is not enabled, the activity will be null and thus the faulty code path is not taken.

Looking at the code, the same probably applies to the non-async Connect.

Further looking at the code, the same probably also applies to Pop3Client: https://github.com/jstedfast/MailKit/blob/c39a209f6f196447e069306a13821f3b10b5ab7b/MailKit/Net/Pop3/Pop3Client.cs#L1395-L1398

I didn't repro these other cases.

I don't think this applies to SmtpClient, as it does not have a seperate engine which's Uri property is used. Instead, in this class it's private uri field is properly set before it is used (note there is no var in out uri here):

https://github.com/jstedfast/MailKit/blob/c39a209f6f196447e069306a13821f3b10b5ab7b/MailKit/Net/Smtp/SmtpClient.cs#L1669-L1671

Platform (please complete the following information):

Exception

System.NullReferenceException: Object reference not set to an instance of an object.
   at MailKit.Net.NetworkOperation..ctor(NetworkOperationKind kind, Uri uri, Activity activity, ClientMetrics metrics)
   at MailKit.Net.NetworkOperation.Start(NetworkOperationKind kind, Uri uri, ActivitySource source, ClientMetrics metrics)
   at MailKit.Net.Imap.ImapEngine.StartNetworkOperation(NetworkOperationKind kind)
   at MailKit.Net.Imap.ImapClient.ConnectAsync(String host, Int32 port, SecureSocketOptions options, CancellationToken cancellationToken)
   at Program.<<Main>$>g__ConnectAsync|0_0() in /__w/MailKitTelemetryNre/MailKitTelemetryNre/Program.cs:line [12](https://github.com/georg-jung/MailKitTelemetryNre/actions/runs/9776775182/job/26990023687#step:4:13)
   at Program.<Main>$(String[] args) in /__w/MailKitTelemetryNre/MailKitTelemetryNre/Program.cs:line 28
   at Program.<Main>(String[] args)

To Reproduce Full repro in GitHub Actions: https://github.com/georg-jung/MailKitTelemetryNre/actions/runs/9776775182/job/26990023687

Otherwise, see my repro repo MailKitTelemetryNre or (expecting you have a reachable IMAP server at hostname greenmail and port 3143):

using System.Diagnostics;
using MailKit.Net.Imap;
using OpenTelemetry;
using OpenTelemetry.Trace;

async Task ConnectAsync()
{
    using var c = new ImapClient();
    await c.ConnectAsync("greenmail", 3143, false);
}

await ConnectAsync(); // this works
Console.WriteLine("Connected to greenmail");

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    .AddSource("demo")
    .AddSource(MailKit.Telemetry.ImapClient.ActivitySourceName)
    .AddConsoleExporter()
    .Build();

var s = new ActivitySource("demo");
s.StartActivity("test");
Console.WriteLine("Activity started");

await ConnectAsync(); // this doesn't
Console.WriteLine("Connected to greenmail");

Expected behavior No NRE is thrown if ImapClient or Pop3Client are used with OTEL traces enabled.

Code Snippets Included above.

Protocol Logs n/a

jstedfast commented 2 days ago

Oof! Thanks for the bug report!