kmaragon / Konscious.Security.Cryptography

MIT License
202 stars 20 forks source link

Prevent the deadlock in Asp.net or UI. #15

Closed GhostTW closed 7 years ago

GhostTW commented 7 years ago

Thanks for the implement Argon2.

I am using Argon2 on my Asp.net WebApi2 project for encrypt string. And I found my request will stuck after WhenAll method, after debug and some search on Google. I found this is similar to mine. Try to fix it and add a unit test.

kmaragon commented 7 years ago

Can I ask why you can't instead use

GetBytesAsync("#S3cr3t+Ke4$").ConfigureAwait(false)

In the ASP.NET application

Rather than changing the library in a way that the user can't control?

GhostTW commented 7 years ago

You are right, I should not change the library that user can't control.

sorry for bad PR and thanks again.

GhostTW commented 7 years ago

hmm

I tried to rewrite but still stuck in the first await Task.WhenAll(init);


public IHttpActionResult A(string data)
{
   var result =Argon2(data, "1234").Result;
   ...
}

public static async Task<string> Argon2(string password, string salt)
{
   ...
   var hash = await argon2.GetBytesAsync(resultLength).ConfigureAwait(false);
   ...
   return Convert.ToBase64String(hash);
}
kmaragon commented 7 years ago

So I think that's actually just recreating the first issue but now directly in the controller rather than inside the Argon2 library call. Now the thing that needs to configure await is the Argon2() method call task. I think your two options are:

A)

public async Task<IHttpActionResult> A(string data)
{
   var result = await Argon2(data, "1234");
   ...
}

(Note that going this way would actually have avoided the other issue altogether) Or B)

public IHttpActionResult A(string data)
{
    var task = Argon2(data, "1234").ConfigureAwait(false);
    var result = task.Result;
    ...
}

Following ASP.NET guidance, it's better if the controller action itself be a Task which ASP.NET manages correctly (probably by just doing the call to ConfigureAwait itself but I didn't actually go check the source for what it does). Otherwise, any control flow chain that synchronously waits for a task in an action needs to ConfigureAwait on the outer-most synchronous wait.

This is actually an issue with ASP.NET in general. There's not actually anything special that the Argon2 library is doing other than starting tasks and waiting for them and this is the specific thing that's problematic in ASP.NET. But of course, with the call to GetBytes, the fact that there's a synchronous Task wait is kind of hidden. I should probably document that better. You're the second person to run into that (the first one is what prompted the creation of GetBytesAsync). I would have preferred to leave the async method as the only call. Unfortunately, doing so wouldn't satisfy the base class contract.

kmaragon commented 7 years ago

I thought I would add for some clarity:

The default behavior for the task would be theoretically: Thread 1: issue work (create tasks) Thread 2: Start a Task Thread 3: Start a Task -- placeholder Thread 2: Finish Task Thread 3: Finish Task + Start a continuation via (Task.WhenAll.ContinueWith for example) Thread 3: Finish continuation At this point, if ConfigureAwait(true) and the synchronization context is "special" Thread 3: updates a data structure to tell Thread 1 to hop back onto it's continuation when it's ready to switch contexts Otherwise Perhaps Thread 3: picks up where Thread 1 left off before since it's in a a good spot to do so. Or maybe not, maybe some other thread that's available. Regardless, the thread that resumes where thread1 left off will be the next available thread. But we'll say: Thread 3: Grab the state from where Thread 1 left off and run the continuation after the await.

In the first case, there's a problem if the synchronization context is set up so that where "--placeholder" is up there, Thread 1 starts waiting on the task. Because then when Thread 3 finishes and tells Thread 1 to continue where it left off, thread 1 is busy waiting for the task to complete so it can't pick up the task that just completed.

This screwy synchronization context happens when you are in a Windows Desktop app and start the task in the UI thread or if you are on ASP.NET starting the task from the controller action thread. In he UI case, it ensures that you can have a chain of async awaits and not suddenly have another thread simultaneously trying to update the UI.

In the Web case, it's a similar problem but now it's an IIS thread that was loaned out to your ASP.NET pipeline and much of what you know as HttpContext is derived from that. So with ConfigureAwait(false), you might find that HttpContext.Current is suddenly a totally different context or outright non-existent. So it's definitely something to use with caution even in this scenario.

GhostTW commented 7 years ago

Really appreciate for your detailed replied !

I will try it these day!