Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt by pjanotti · Pull Request #29186 · dotnet/corefx

pjanotti · 2018-04-18T17:51:52Z

[EDIT: updating the names to the ones that were actually merged]

This is an escape valve in case the optimization of removing the calls to ERR_clear_error() for each SSL_write/SSL_read causes troubles. There will be a performance impact but depending on the scenario it can be a desired trade-off.

The AppContext switch is "System.Net.Security.SslStream.ForceClearOpenSslErrorQueue"
The env var is "DOTNET_SYSTEM_NET_SECURITY_SSLSTREAM_FORCECLEAROPENSSLERRORQUEUE"

This is a escape valve in case the optimization of removing the calls to ERR_clear_error() for each SSL_write/SSL_read causes troubles. There will be a performance impact but depending on the scenario it can be a desired trade-off.

karelz · 2018-04-18T18:17:43Z

@pjanotti fixing the milestone - it will go into master = 2.2. If we want it to 2.1, we need to cherry pick it + it would be good to have a tracking bug. Can you please create one?

pjanotti · 2018-04-18T18:46:49Z

Done, #29188 created.

pjanotti · 2018-04-18T20:25:30Z

@dotnet/dnceng OSX x64 seems stuck https://ci3.dot.net/job/dotnet_corefx/job/master/job/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/11570/consoleText after successful build.

@dotnet-bot test OSX x64 Debug Build

mmitche · 2018-04-18T20:28:03Z

@pjanotti Not stuck, just backed up.

pjanotti · 2018-04-18T20:29:43Z

Ops, then I didn't help by asking for another build...

stephentoub · 2018-04-18T21:49:27Z

+            if (!AppContext.TryGetSwitch("System.Net.Security.SslStream.ForceOpenSslErrorQueueCleanupBeforeEncryptDecrypt", out bool forceErrorQueueCleanup))
+            {
+                // AppContext wasn't used, try the environment variable.
+                string envVar = Environment.GetEnvironmentVariable("DOTNET_FORCE_OPENSSL_ERROR_QUEUE_CLEANUP_BEFORE_ENCRYPT_DECRYPT");


In general I think we should keep the appctx switch name and the env var name to be as similar as possible, e.g.
System.Net.Security.SslStream.ForceClearOpenSslErrorQueue
and
DOTNET_SYSTEM_NET_SECURITY_SSLSTREAM_FORCECLEAROPENSSLERRORQUEUE

stephentoub · 2018-04-18T21:50:35Z

+
 extern "C" int32_t CryptoNative_SslWrite(SSL* ssl, const void* buf, int32_t num)
 {
+    if (g_forceOpenSslErrorQueueCleanupBeforeWriteRead)


Does this check/branch have any impact on the case where it's not set? i.e. the default case where the user doesn't set the flag? Presumably it's a drop in the bucket compared to the comparison, but it'd be good to know for sure.

The performance with the config (default, optimization on) is indistinguishable compared to master - same test indicates that when the optimization is off there is a 5 to 15% extra time to complete the test (test based on your ConcurrentHttpsGet test, but I had to run it under Xunit since I couldn't get Benchmark.NET working against my local build).

stephentoub · 2018-04-18T21:51:26Z

        private unsafe static readonly Ssl.SslCtxSetAlpnCallback s_alpnServerCallback = AlpnServerSelectCallback;
        private static readonly IdnMapping s_idnMapping = new IdnMapping();

+        static OpenSsl()


Before this change, the compiler could make the Interop.OpenSsl class beforefieldinit. Adding this explicit static cctor is going to stop that from happening, which can have perf implications.

Good point, perhaps initialize a field with a side effect? Let me know if you have any suggestions

stephentoub · 2018-04-18T21:53:04Z

How far are we from having audited all of our calls? This may be naive, but it seems like the right answer is to audit all of the calls, make sure we're doing the right thing, and then we don't need a flag like this. If someone else in the process is using OpenSSL incorrectly, that's then their problem.

cc: @bartonjs

maririos · 2018-04-18T22:05:11Z

@pjanotti Not stuck, just backed up.

Issue that is tracking the work to enable more OSX is https://github.com/dotnet/core-eng/issues/3284

pjanotti · 2018-04-18T22:22:21Z

How far are we from having audited all of our calls?

I'm starting this work, first I wanted to address the ones that the debugger showed that were explicit putting errors in the queue in order to reduce noise. I should have a ball-park estimate for the amount of work by tomorrow.

We have to audit our calls but this is playing defense and having an option if something passes our combing.

bartonjs

Aside from the style comments, I question the purpose.

If the purpose is: "Leaked error states cause exceptions to be thrown during successful cases, and this helps applications move onto 2.1", then OK.

If the purpose is "the wrong exception is thrown, but it was throwing anyways, this makes it be the right exception" then I don't know it's useful enough to warrant a) change, and b) maintenance of that change.

bartonjs · 2018-04-19T15:34:53Z

 extern "C" int32_t CryptoNative_SslRead(SSL* ssl, void* buf, int32_t num)
 {
+    if (g_forceOpenSslErrorQueueCleanupBeforeWriteRead)
+        ERR_clear_error();


Please add the explicit braces. The only things in this file (and probably most of this library) which use an unbraced if are stacked if->returns

bartonjs · 2018-04-19T15:37:05Z

            out int hashKeySize);

+        [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_ForceOpenSslErrorQueueCleanupBeforeWriteRead")]
+        internal static extern void ForceOpenSslErrorQueueCleanupBeforeWriteRead();


OpenSsl in the name is a bit redundant

bartonjs · 2018-04-19T15:37:34Z

+// Controls if ERR_clear_error is going to be called before SSL_write/SSL_read
+static bool g_forceOpenSslErrorQueueCleanupBeforeWriteRead = false;
+
+extern "C" void CryptoNative_ForceOpenSslErrorQueueCleanupBeforeWriteRead()


All of our extern functions should also be defined in the corresponding header.

No problem, I will follow it for consistency, but I'm a bit curious about the purpose of it. Do you know why?

pjanotti · 2018-04-19T16:28:04Z

Leaked error states cause exceptions to be thrown during successful cases, and this helps applications move onto 2.1

Yes, this is the issue being addressed here.

We don't need this if we can be almost sure that we ourselves don't leave any errors on the queue. My initial count shows that we will have to review about 250 native calls, hopefully most of them should be trivial, but anyway this needs to be done carefully (both to not to regress perf nor lose error information).

pjanotti · 2018-04-20T06:06:27Z

Failures on Ubuntu 14.04 per inspection of a random set seems related to sockets:

System.Net.Sockets.SocketException : Cannot assign requested address

@dotnet-bot test Outerloop Linux x64 Release Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Linux x64 Release Build
@dotnet-bot test Linux x64 Debug Build

dotnet#29186) * Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt This is a escape valve in case the optimization of removing the calls to ERR_clear_error() for each SSL_write/SSL_read causes troubles. There will be a performance impact but depending on the scenario it can be a desired trade-off. Fixes #29188

…t/Decrypt (dotnet#29186)" This reverts commit 2750fdd.

…t/Decrypt (#29186)" (#29528) This reverts commit 2750fdd.

dotnet/corefx#29186) * Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt This is a escape valve in case the optimization of removing the calls to ERR_clear_error() for each SSL_write/SSL_read causes troubles. There will be a performance impact but depending on the scenario it can be a desired trade-off. Fixes dotnet/corefx#29188 Commit migrated from dotnet/corefx@2750fdd

…t/Decrypt (dotnet/corefx#29186)" (dotnet/corefx#29528) This reverts commit dotnet/corefx@2750fdd. Commit migrated from dotnet/corefx@3f5d26c

pjanotti added the area-System.Security label Apr 18, 2018

pjanotti added this to the 2.1.0 milestone Apr 18, 2018

pjanotti self-assigned this Apr 18, 2018

pjanotti requested review from bartonjs, eerhardt, karelz and stephentoub April 18, 2018 17:51

karelz modified the milestones: 2.1.0, 2.2.0 Apr 18, 2018

stephentoub reviewed Apr 18, 2018

View reviewed changes

bartonjs reviewed Apr 19, 2018

View reviewed changes

PR Feedback 00

b0299a4

pjanotti merged commit 2750fdd into dotnet:master Apr 21, 2018

pjanotti deleted the config.force.openssl.err.cleanup branch April 21, 2018 02:43

pjanotti mentioned this pull request May 1, 2018

Port "Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt" to 2.1 #29443

Closed

pjanotti pushed a commit to pjanotti/corefx that referenced this pull request May 4, 2018

Revert "Add config to force OpenSsl error queue cleanup before Encryp…

a4c044f

…t/Decrypt (dotnet#29186)" This reverts commit 2750fdd.

pjanotti pushed a commit that referenced this pull request May 7, 2018

Revert "Add config to force OpenSsl error queue cleanup before Encryp…

3f5d26c

…t/Decrypt (#29186)" (#29528) This reverts commit 2750fdd.

pjanotti mentioned this pull request Jan 31, 2020

Add config to force OpenSsl error queue cleanup before Encrypt/Decrypt dotnet/runtime#25930

Closed

Uh oh!

Conversation

pjanotti commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karelz commented Apr 18, 2018

Uh oh!

pjanotti commented Apr 18, 2018

Uh oh!

pjanotti commented Apr 18, 2018

Uh oh!

mmitche commented Apr 18, 2018

Uh oh!

pjanotti commented Apr 18, 2018

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!

stephentoub commented Apr 18, 2018

Uh oh!

maririos commented Apr 18, 2018

Uh oh!

pjanotti commented Apr 18, 2018

Uh oh!

bartonjs left a comment

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!

pjanotti commented Apr 19, 2018

Uh oh!

pjanotti commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pjanotti commented Apr 18, 2018 •

edited

Loading