Always clear the local buffer in ArrayBuffer #49573

davidfowl · 2021-03-13T07:32:44Z

SslStream was holding onto a 4K byte[] after the handshake was complete. This was because the ArrayBuffer struct doesn't clear the local buffer field in dispose. This changes that.

Before:

After:

Blocked on #49945

ghost · 2021-03-13T07:32:51Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2021-03-13T07:33:29Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

SslStream was holding onto a 4K byte[] after the handshake was complete. This was because the ArrayBuffer struct doesn't clear the local buffer field in dispose. This changes that.

Before:

After:

Author:	davidfowl
Assignees:	-
Labels:	`area-System.Net.Security`
Milestone:	-

wfurt

LGTM

src/libraries/Common/src/System/Net/ArrayBuffer.cs

davidfowl · 2021-03-13T14:49:55Z

Are these tests generally broken?

stephentoub · 2021-03-13T15:07:31Z

The mono ones are #49569.

The coreclr osx ones I haven't seen. Can you open an issue for that?

CI has been very unhappy the last few days. I suspect as a result things were merged that shouldn't have been, compounding the problem.

davidfowl · 2021-03-13T15:28:31Z

Looks like a timezone thing.

davidfowl · 2021-03-13T15:32:22Z

this fix actually broke a test:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Net.ArrayBuffer.get_AvailableLength() in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 58
   at System.Net.ArrayBuffer.EnsureAvailableSpace(Int32 byteCount) in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 86
   at System.Net.SafeDeleteSslContext.WriteToConnection(Void* connection, Byte* data, Void** dataLength) in /_/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs:line 164
./RunTests.sh: line 162: 23722 Abort trap: 6           (core dumped) "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Net.Security.Tests.runtimeconfig.json --depsfile System.Net.Security.Tests.deps.json xunit.console.dll System.Net.Security.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/p

davidfowl · 2021-03-13T15:45:03Z

~~this fix actually broke a test:~~ Uncovered a user after free bug on OSX.

Looks like something is calling WriteToConnection after Dispose @wfurt ?

wfurt · 2021-03-13T19:16:37Z

This is similar to #43686. We really should throw ObjectDisposed exception in this case. HttpClient with H2 is particularly prune to write after Dispose. I'll check if I can reproduce it locally with your change @davidfowl. I could not reproduce #43686 so it got eventually closed without resolution.

I still feel the change make sense. We will just need to improve error checking inside SslStream. (or hold off like we do for the _nestedRead)

davidfowl · 2021-03-13T20:28:11Z

Yes, the change does make sense. I was looking into the SSL APIs on OSX but I think that should be fixed separately. I'm gonna merge this, can you open an issue the write after dispose?

stephentoub · 2021-03-13T20:31:21Z

I'm gonna merge this

If merging this is going to tank CI further, please don't.

davidfowl · 2021-03-13T20:58:40Z

OK I pushed a new change that tries to keep the old behavior (writing into the ArrayBuffer after dispose), which should allow some slack for fixing the real use after free issue. We can revert that part of the change when we figure out why its happening on OSX.

stephentoub · 2021-03-13T21:01:54Z

This change isn't critical to get in asap. Let's just prioritize finding and fixing the problem and hold on this until we do.

davidfowl · 2021-03-13T21:11:09Z

I'm all for fixing the bug I don't want to hold this change hostage though. While it's not critical to get in ASAP, my only fear is that the bug will get punted and also the fix. At a bare minimum, if the fix for the real issue isn't trivial or the fix is hard verify (because reproducing is hard), we should take this improvement but I'll let @wfurt decide on what's best.

Good news is that there's a dump captured as part of the crash...

stephentoub · 2021-03-13T21:19:58Z

I don't want to hold this change hostage though

This change is causing a crash. While it should be valid, because of another issue it's not. We should fix the other issue before trying to push this in. And I do not want to complicate the code trying to mask the other issue. As I said, let's prioritize finding and fixing the other one, then this change will be valid and the original simple change can be merged.

davidfowl · 2021-03-13T21:35:21Z

We generally agree, I just want to make sure we have some trip wires and backup plans in place (the other similar issue did get closed). But I'll be optimistic 😄

wfurt · 2021-03-14T21:12:35Z

I think the real issue lives macOS PAL. I can reproduce it locally and I'll take look next week.
I'm not sure we should pollute ArrayBuffer to compensate that. I liked the first simple change better.
cc: @geoffkizer as original author of ArrayBuffer

davidfowl · 2021-03-14T21:34:06Z

Let me know if you want me to revert to the original change or if you want to take over this change in general and pair it with the OSX fix

jkotas · 2021-03-14T21:47:51Z

The workaround in the ArrayBuffer does not look 100% reliable due to race conditions. It will just make the crash to show up less often. Does it sound right? If it is the case, we should definitely wait for the proper fix.

davidfowl · 2021-03-14T21:51:05Z

I'm not defending the workaround for the crash, it's as unreliable as it was before. The ArrayBuffer isn't thread safe without or without this change.

geoffkizer · 2021-03-14T22:05:25Z

As others have said, let's not merge a temporary work-around here. Let's fix the issue in thee OSX PAL or wherever and then merge the proper fix.

my only fear is that the bug will get punted and also the fix.

You've identified a nontrivial perf issue here, and the fix is straightforward. The odds of this getting punted are very low.

davidfowl · 2021-03-15T00:28:51Z

Sounds fine to me

geoffkizer · 2021-03-29T01:02:22Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-03-29T01:02:41Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2021-03-29T01:06:32Z

/azp run

azure-pipelines · 2021-03-29T01:06:37Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

davidfowl · 2021-03-29T01:06:52Z

/azp run runtime

azure-pipelines · 2021-03-29T01:07:16Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2021-03-29T01:07:35Z

/azp run all

azure-pipelines · 2021-03-29T01:07:39Z

No pipelines are associated with this pull request.

davidfowl · 2021-03-29T01:07:54Z

/azp list

azure-pipelines · 2021-03-29T01:07:59Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress runtime-coreclr libraries-gcstress0x3-gcstress0xc runtime-coreclr libraries-gcstress-extra dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr clrinterpreter runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-richnav

davidfowl · 2021-03-29T01:08:31Z

/azp run runtime-dev-innerloop

azure-pipelines · 2021-03-29T01:08:41Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2021-03-29T01:09:01Z

/azp run runtime-staging

azure-pipelines · 2021-03-29T01:09:16Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2021-03-29T01:12:55Z

How do I re-run these mono tests 😄

davidfowl · 2021-03-29T01:13:13Z

/azp run runtime-libraries-mono outerloop

azure-pipelines · 2021-03-29T01:13:24Z

Azure Pipelines successfully started running 1 pipeline(s).

- SslStream was holding onto a 4K byte[] after the handshake was complete. This was because the ArrayBuffer struct doesn't clear the local buffer field in dispose. This changes that.

davidfowl · 2021-03-29T01:17:26Z

Force push never fails...

davidfowl requested review from stephentoub, wfurt and geoffkizer March 13, 2021 07:32

davidfowl added the area-System.Net.Security label Mar 13, 2021

wfurt approved these changes Mar 13, 2021

View reviewed changes

src/libraries/Common/src/System/Net/ArrayBuffer.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Mar 13, 2021

View reviewed changes

davidfowl mentioned this pull request Mar 14, 2021

Use after dispose bug in SafeDeleteSslContext.WriteToConnection on OSX #49600

Closed

runfoapp bot mentioned this pull request Mar 15, 2021

Mono System.Text.Json.Tests on Windows timing out #42677

Open

wfurt mentioned this pull request Mar 15, 2021

delay disposal of array buffres if operation is in progress #49657

Closed

stephentoub approved these changes Mar 29, 2021

View reviewed changes

davidfowl removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 29, 2021

davidfowl closed this Mar 29, 2021

davidfowl reopened this Mar 29, 2021

davidfowl closed this Mar 29, 2021

davidfowl reopened this Mar 29, 2021

Always clear the local buffer in ArrayBuffer

c1a7d45

- SslStream was holding onto a 4K byte[] after the handshake was complete. This was because the ArrayBuffer struct doesn't clear the local buffer field in dispose. This changes that.

davidfowl force-pushed the davidfowl/handshake-buffer branch from 2ce6a3c to c1a7d45 Compare March 29, 2021 01:17

davidfowl merged commit 102d1e8 into main Mar 29, 2021

davidfowl deleted the davidfowl/handshake-buffer branch March 29, 2021 14:16

davidfowl added this to the 6.0.0 milestone Apr 2, 2021

ghost locked as resolved and limited conversation to collaborators May 3, 2021

Always clear the local buffer in ArrayBuffer #49573

Always clear the local buffer in ArrayBuffer #49573

Uh oh!

Conversation

davidfowl commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2021

Uh oh!

ghost commented Mar 13, 2021

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

stephentoub commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

wfurt commented Mar 13, 2021

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

stephentoub commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

stephentoub commented Mar 13, 2021

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

stephentoub commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Mar 13, 2021

Uh oh!

wfurt commented Mar 14, 2021

Uh oh!

davidfowl commented Mar 14, 2021

Uh oh!

jkotas commented Mar 14, 2021

Uh oh!

davidfowl commented Mar 14, 2021

Uh oh!

geoffkizer commented Mar 14, 2021

Uh oh!

davidfowl commented Mar 15, 2021

Uh oh!

geoffkizer commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 29, 2021

Uh oh!

azure-pipelines bot commented Mar 29, 2021

Uh oh!

davidfowl commented Mar 13, 2021 •

edited

Loading

stephentoub commented Mar 13, 2021 •

edited

Loading

stephentoub commented Mar 13, 2021 •

edited

Loading

stephentoub commented Mar 13, 2021 •

edited

Loading