Annotate `System.Diagnostics.EventLog` with Nullable Reference Types #119891

RenderMichael · 2025-09-19T12:44:04Z

Contributes to #90400

dotnet-policy-service · 2025-09-19T12:45:07Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-eventlog
See info in area-owners.md if you want to be subscribed.

xtqqczze · 2025-09-19T20:53:07Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLog.cs


                    bool createLogKey = (logKey == null);
-                    if (createLogKey)
+                    if (logKey == null)


unnecessary change

Maybe this is a hole in Roslyn’s nullability analysis, but the analyzer warns about maybe-null logKey otherwise

I see, in that case it would be clearer if we said bool createLogKey = false, then createLogKey = true inside the condition.

That's a great idea.

xtqqczze · 2025-09-19T20:58:16Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLog.cs

                try
                {
-                    key = FindSourceRegistration(source, machineName, false);
+                    key = FindSourceRegistration(source, machineName, false)!;


there might be a bug here if key can be null

We validate that it’s not null because we make this same call on line 450 (and throw if the value turned out null)

I'm not sure, the method makes a call to interop, the registry could change in the meantime. It seems like a simple fix (maybe not in this PR) to use null-conditional operator in line 470.

The code as-is definitely makes the assumption that the registry doesn’t change. Given the validation just above, key == null here seems like an error state that we should throw in instead of tolerate. A NRE is an unpleasant version of just that, though we could certainly make the exception better.

I think for the annotation PR the NRE is fine

xtqqczze · 2025-09-19T21:09:33Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLog.cs

                if (!string.IsNullOrEmpty(assmLocation))
                {
-                    dllPath = Path.Combine(Path.GetDirectoryName(assmLocation), AltDllName);
+                    dllPath = Path.Combine(Path.GetDirectoryName(assmLocation)!, AltDllName);


looks like a bug, maybe we should fix this in a separate PR?

string? directoryName = Path.GetDirectoryName(assmLocation); if (directoryName is not null) { dllPath = Path.Combine(directoryName, AltDllName); }

Path.GetDirectoryName is documented to return null if the path denotes a root directory or is null. Is it possible to make Assembly.Location point to a root directory?

Since it seems like an impossible state to me, maybe we could Debug.Assert on it instead of creating a branch that's hard to validate?

xtqqczze · 2025-09-19T21:15:02Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

        }

-        public EventLogInternal(string logName, string machineName) : this(logName, machineName, "", null)
+        public EventLogInternal(string logName, string machineName) : this(logName, machineName, "", null! /* Special case with no parent */)


I don't think this is correct

The EventLogInternal(string, string) constructor is only called in EventLogInternal.AddListenerComponent and stored in an LogListeningInfo. Only the ReadHandle member is read on this EventLogInternal for the duration of its lifetime, until it is removed in EventLogInternal.RemoveListenerComponent. EventLogInternal.parent is never called.

The other "special" constructor is called on various overloads of EventLog.WriteEntry, where it is immediately used to call WriteEvent (a method that also doesn't touch parent) and then disposed.

Maybe this type could use some "separation of concerns" refactoring, or maybe we should assert that parent is not null (it's assumed to be not-null on each callsite). I'm happy to change this from a null-suppression at the ctor to some other shape.

I my opinion, the ctor should be refactored, so that parent is left uninitialised in the "special" cases. The majority of the logic would be moved to EventLogInternal(string logName, string machineName, string source). So the last ctor would look like:

public EventLogInternal(string logName, string machineName, string source, EventLog parent) : this(logName, machineName, source) { this.parent = parent; }

The field can then be declared as:

[DisallowNull] private readonly EventLog? parent;

We would need null-suppression at the call-sites though.

We can narrow this down even to a single constructor; added null-suppression at the call-sites like you mentioned.

xtqqczze · 2025-09-19T21:15:16Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

        }

-        public EventLogInternal(string logName, string machineName, string source) : this(logName, machineName, source, null)
+        public EventLogInternal(string logName, string machineName, string source) : this(logName, machineName, source, null! /* Special case with no parent */)


I don't think this is correct

Ditto #119891 (comment)

xtqqczze · 2025-09-19T21:17:50Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

                        else
                        {
-                            int resourceId = (int)logkey.GetValue("DisplayNameID");
+                            int resourceId = (int)logkey.GetValue("DisplayNameID")!;


we should probably handle the null case

That's a good point.

All other calls to RegistryKey.GetValue handle null, though they are all reference types.

The LogDisplayName property seems to be "best-effort", trying a bunch of different ways to get some decent display name. In other words, we likely shouldn't be throwing if DisplayNameID is not present.

xtqqczze · 2025-09-19T21:18:58Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs


-                info = new LogListeningInfo();
+
+                info = new LogListeningInfo(


unrelated changes

This gives us definite not-null assignment.

xtqqczze · 2025-09-19T21:22:48Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

                        EventLogEntry entry = GetEntryWithOldest(i);
                        if (this.SynchronizingObject != null && this.SynchronizingObject.InvokeRequired)
-                            this.SynchronizingObject.BeginInvoke(this.onEntryWrittenHandler, new object[] { this, new EntryWrittenEventArgs(entry) });
+                            this.SynchronizingObject.BeginInvoke(this.onEntryWrittenHandler!, new object[] { this, new EntryWrittenEventArgs(entry) });


maybe this should be fixed

I went into this not knowing how much leeway I have to change any behavior; I set out to faithfully represent any holes in nullability. This is definitely one of those holes, probably hidden by this nerve-racking line:

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

Lines 573 to 575 in 2b01031

catch (Exception)

{

}

Added a null check. This event is ultimately exposed with the EventLog.EntryWritten event; handled the scenario where nobody subscribes to it.

xtqqczze · 2025-09-19T21:43:47Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs


        private EventLogEntry GetEntryWithOldest(int index)
        {
+            Debug.Assert(readHandle != null);


how do we know these invariants hold?

There are two callsites: GetEntryAtNoThrow (which opens a readHandle first thing) and CompletionCallback (which calls OldestEntryNumber's getter earlier; OldestEntryNumber opens a readHandle unconditionally).

The assert on cache should be within the if (entryPos >= 0) block, fixing that.

xtqqczze · 2025-09-19T21:44:20Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs


        private int GetNextEntryPos(int pos)
        {
+            Debug.Assert(cache != null);


how do we know these invariants hold?

Both callsites for this method are within GetCachedEntryPos, which early-exits if cache == null

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

Lines 769 to 777 in 2b01031

private int GetCachedEntryPos(int entryIndex)

{

if (cache == null || (boolFlags[Flag_forwards] && entryIndex < firstCachedEntry) ||

(!boolFlags[Flag_forwards] && entryIndex > firstCachedEntry) || firstCachedEntry == -1)

{

// the index falls before anything we have in the cache, or the cache

// is not yet valid

return -1;

}

xtqqczze · 2025-09-19T21:44:32Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs


        private int GetPreviousEntryPos(int pos)
        {
+            Debug.Assert(cache != null);


how do we know these invariants hold?

All 3 callsites for this method are within GetCachedEntryPos, which early-exits if cache == null

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

Lines 769 to 777 in 2b01031

private int GetCachedEntryPos(int entryIndex)

{

if (cache == null || (boolFlags[Flag_forwards] && entryIndex < firstCachedEntry) ||

(!boolFlags[Flag_forwards] && entryIndex > firstCachedEntry) || firstCachedEntry == -1)

{

// the index falls before anything we have in the cache, or the cache

// is not yet valid

return -1;

}

xtqqczze · 2025-09-19T21:46:55Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

                info.handleOwner.Dispose();
                //Unregister the thread pool wait handle
-                info.registeredWaitHandle.Unregister(info.waitHandle);
+                info.registeredWaitHandle?.Unregister(info.waitHandle);


just thought I'd call out the added null check here

I think I was being overly cautious here, registeredWaitHandle is set in the initial AddListenerComponent. Reverting.

xtqqczze · 2025-09-19T21:53:42Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

-                        strings[i] = values[i].ToString();
-                    else
-                        strings[i] = string.Empty;
+                    strings[i] = values[i]?.ToString() ?? string.Empty;


this is a change in behavior, previously strings[i] could be null

strings[i] could technically be null by virtue of object.ToString() returning a nullable string. The original code does a null check on values[i] and assigns string.Empty

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

Lines 1333 to 1336 in 2b01031

if (values[i] != null)

strings[i] = values[i].ToString();

else

strings[i] = string.Empty;

This change both simplifies the code and works around a potentially null-returning ToString() method. However, It seems to me that the original devs did not want a null value in this array.

xtqqczze · 2025-09-19T21:56:00Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

                // make sure the strings aren't too long.  MSDN says each string has a limit of 32k (32768) characters, but
                // experimentation shows that it doesn't like anything larger than 32766
-                if (strings[i].Length > 32766)
+                if (strings[i]!.Length > 32766)


related to above, we need to decide if strings is string?[]? or string[]?

The original devs were overly cautious about deliberately preventing a string?[]? scenario, as per the line just above

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs

Line 1352 in 2b01031

strings[i] ??= string.Empty;

xtqqczze · 2025-09-19T21:56:53Z

src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLogInternal.cs


        private sealed class LogListeningInfo
        {
+            public LogListeningInfo(EventLogInternal handleOwner, AutoResetEvent waitHandle)


Is adding the constructor necessary for the null annotation work?

Yes, it's this or = null!; on each field. I prefer this approach because it works with Roslyn instead of around Roslyn.

…callsites

…ce run

RenderMichael added 2 commits September 19, 2025 08:34

Annotate System.Diagnostics.EventLog with Nullable Reference Types

adc1f73

Remove compatibility suppressions, build seems to work without them now

67aaffc

github-actions bot added the area-System.Diagnostics.EventLog label Sep 19, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 19, 2025

xtqqczze reviewed Sep 19, 2025

View reviewed changes

RenderMichael added 5 commits September 19, 2025 20:01

Initial PR feedback

583ebbf

PR feedback

949f417

Address scenario where EventLog.EntryWritten event is not subscribed to

c35e76e

Move null suppression from EventLogInternal constructor overloads to …

950bf63

…callsites

Add back in attributes deleted from the GenerateReferenceAssemblySour…

a06202c

…ce run

	private int GetCachedEntryPos(int entryIndex)
	{
	if (cache == null \|\| (boolFlags[Flag_forwards] && entryIndex < firstCachedEntry) \|\|
	(!boolFlags[Flag_forwards] && entryIndex > firstCachedEntry) \|\| firstCachedEntry == -1)
	{
	// the index falls before anything we have in the cache, or the cache
	// is not yet valid
	return -1;
	}

	if (values[i] != null)
	strings[i] = values[i].ToString();
	else
	strings[i] = string.Empty;

Annotate System.Diagnostics.EventLog with Nullable Reference Types #119891

Are you sure you want to change the base?

Annotate System.Diagnostics.EventLog with Nullable Reference Types #119891

Conversation

RenderMichael commented Sep 19, 2025

Uh oh!

dotnet-policy-service bot commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annotate `System.Diagnostics.EventLog` with Nullable Reference Types #119891

Annotate `System.Diagnostics.EventLog` with Nullable Reference Types #119891

xtqqczze Sep 19, 2025 •

edited

Loading

xtqqczze Sep 19, 2025 •

edited

Loading