Skip to content

Handle null reference exception when TicketCacheBase.Refresh is null#372

Merged
SteveSyfuhs merged 3 commits into
dotnet:developfrom
TimHess:null_refresh
Jul 5, 2024
Merged

Handle null reference exception when TicketCacheBase.Refresh is null#372
SteveSyfuhs merged 3 commits into
dotnet:developfrom
TimHess:null_refresh

Conversation

@TimHess

@TimHess TimHess commented Jul 3, 2024

Copy link
Copy Markdown
Contributor

What's the problem?

A NullReferenceException can happen when using Krb5TicketCache.

  • Bugfix
  • New Feature

What's the solution?

Evaluate whether Refresh is null before trying to Invoke it and .ConfigureAwait(false).

  • Includes unit tests
  • Requires manual test

Background information

I inherited some code that hits a NullReferenceException when the cache tries to refresh. I struggled to write a test for this scenario due to the way the different objects (and default values) are instantiated, the methods being private and the nature of the background tasks involved.

I'm still getting up to speed on everything involved, but I believe the most relevant bits I'm starting from include something like this:

var client = new KerberosClient(Krb5Config.Default());
client.CacheInMemory = false;
client.Cache = new Krb5TicketCache("path to cache file");
client.RenewTickets = true;

Comment thread Kerberos.NET/Cache/TicketCacheBase.cs Outdated
await (this.Refresh?.Invoke(cacheEntry)).ConfigureAwait(false);
if (this.Refresh != null)
{
await this.Refresh.Invoke(cacheEntry).ConfigureAwait(false);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually regresses to a slightly more problematic issue because Refresh can be nulled out between the null check and the call across threads, whereas this.Refresh?.Invoke ends up being an atomic operation. As such, I think the fix is actually simpler: make it await (this.Refresh?.Invoke(cacheEntry))?.ConfigureAwait(false);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is complaining:

Kerberos.NET\Cache\TicketCacheBase.cs(161,21): Error CS1061: 'ConfiguredTaskAwaitable?' does not contain a definition for 'GetAwaiter' and no accessible extension method 'GetAwaiter' accepting a first argument of type 'ConfiguredTaskAwaitable?' could be found (are you missing a using directive or an assembly reference?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, I suppose that's what I get for editing from my phone! I should be able to take a closer look on Friday

@TimHess TimHess requested a review from SteveSyfuhs July 5, 2024 16:00
@TimHess

TimHess commented Jul 5, 2024

Copy link
Copy Markdown
Contributor Author

The CI failure does not appear to be related to the change as build and tests all passed

Error:
Microsoft.VisualStudio.Services.Common.VssServiceException: VS800075: The project with id 'vstfs:///Classification/TeamProject/bcf490cf-a965-4d26-999c-8de04067ee1e' does not exist, or you do not have permission to access it.
   at Microsoft.VisualStudio.Services.WebApi.VssHttpClientBase.<HandleResponseAsync>d__53.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.Services.WebApi.VssHttpClientBase.<SendAsync>d__51.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.Services.WebApi.VssHttpClientBase.<SendAsync>d__31.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at VstsNuGetPush.PackageAdder.AddPackageToFeed(FeedCoordinates feed, BlobIdentifierWithBlocks blobId, Boolean muted)
   at VstsNuGetPush.VstsNuGetPushBlobStoreOperation.AddPackageToFeed(BlobIdentifierWithBlocks blobId, Boolean muted)
   at VstsNuGetPush.VstsNuGetPushBlobStoreOperation.Execute()
   at VstsNuGetPush.VstsNuGetPushCommand.MainCore(ILogger logger)
   at VstsNuGetPush.VstsNuGetPushCommand.Main()
Trying to authenticate with auth token.
Successfully authenticated.
Authentication and request took 00:00:03.4351189
Adding package Bruce.4.6.61-gd207cb1436.nupkg to feed kerberos.net on [https://syfuhs2.pkgs.visualstudio.com/.](https://syfuhs2.pkgs.visualstudio.com/)
Trying to add package to feed without uploading.
Adding package to feed.

@SteveSyfuhs

Copy link
Copy Markdown
Collaborator

That's the build service being dumb. I need to fix the build step to not try and push PR nuget packages to the private repo internally. Otherwise change looks good.

@SteveSyfuhs SteveSyfuhs merged commit 89ae633 into dotnet:develop Jul 5, 2024
@TimHess TimHess deleted the null_refresh branch July 15, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants