dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

Consider debug mode for ArrayPool #7532

Open danmoseley opened 7 years ago

danmoseley commented 7 years ago

There are several ways to misuse array pool

  1. forget to return - merely inefficient
  2. return and continue to use
  3. renter expects pre cleared array
  4. returner does not request a clear and leaks sensitive data
  5. double return

There is some logging to help trace suspected cases of 1 and 2.

For 4 if we start caring more about that someday we could change the default to clear data.

For 5 we could at least (in a diagnostic mode) compare against existing arrays in the pool (perhaps an expanded size pool that returns LRU so it's likely still in the pool when returned the 2nd time)

We could help catch cases of both 2 and 3 by having a mode where we set the array to 0xDEADBEEF on return. We would do this whether or not the returner requested the array clear, because the next renter cannot rely on the returner.

This should be enabled in #if DEBUG but since running chk corelib is so rarely done, it might realistically only get used if it was #if DEBUG || DEBUG_ARRAYPOOL or something like that. So perhaps this isn't worth bothering with unless and until we have some investigation that it would help with.

Just a thought for the future.

danmoseley commented 7 years ago

cc @stephentoub

jkotas commented 7 years ago

Requiring a different build of the runtime to do this is non-starter. This would need to be enabled dynamically with a config switch or environment variable. Similar to how malloc/free diagnostic works in C++ today (pageheap on Windows, MALLOC_CHECK_ on Linux, etc.)

danmoseley commented 7 years ago

Oh - it's right in front of my nose in PinnableBufferCache already. Just another one.

#if ENABLE
            // Check to see if we should disable the cache.
            string envVarName = "PinnableBufferCache_" + cacheName + "_Disabled";
            try
            {
                string envVar = Environment.GetEnvironmentVariable(envVarName);
                if (envVar != null)
jkotas commented 4 years ago

Related: https://developercommunity.visualstudio.com/content/problem/1027129/memory-corruption-when-pinning-arraypool-arrays.html

danmoseley commented 4 years ago

Another option would be a different factory method to create the ArrayPool (perhaps overloads or instead of Shared() or Create()) that would return the diagnostic pool. Presumably there are relatively few places in one's code where the array pool is created (probably 1, if you know which array pool is leading to the problem) so changing that one line of code for testing purposes is not onerous. As a separate concrete implementation of ArrayPool performance would not be a concern and it could record callstacks, etc like a diagnostic heap.

huoyaoyuan commented 2 months ago

Can this be achieved with AppContext switch? Since the checks may be somehow intensive, I'm not sure about Tier-0 performance impact.