From f3a2224ca86bf246a5134a191e110476b41adcf5 Mon Sep 17 00:00:00 2001 From: Igor Velikorossov Date: Fri, 17 May 2024 11:34:28 +1000 Subject: [PATCH] fixup! Avoid buffer race conditions in CGroups --- .../Linux/LinuxUtilizationParserCgroupV1.cs | 91 +++++++++---------- .../ReturnableBufferWriter.cs | 45 +++++++++ 2 files changed, 88 insertions(+), 48 deletions(-) create mode 100644 src/Shared/BufferWriterPool/ReturnableBufferWriter.cs diff --git a/src/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs b/src/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs index bd926284c8f..a5bc52b089b 100644 --- a/src/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs +++ b/src/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs @@ -4,7 +4,7 @@ using System; using System.Diagnostics.CodeAnalysis; using System.IO; -using System.Threading; +using Microsoft.Extensions.ObjectPool; using Microsoft.Shared.Diagnostics; using Microsoft.Shared.Pools; @@ -18,6 +18,7 @@ namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux; internal sealed class LinuxUtilizationParserCgroupV1 : ILinuxUtilizationParser { private const float CpuShares = 1024; + private static readonly ObjectPool> _sharedBufferWriterPool = BufferWriterPool.CreateBufferWriterPool(); /// /// File contains the amount of CPU time (in microseconds) available to the group during each accounting period. @@ -86,9 +87,6 @@ internal sealed class LinuxUtilizationParserCgroupV1 : ILinuxUtilizationParser private readonly IFileSystem _fileSystem; private readonly long _userHz; - private readonly ThreadLocal> _threadBuffer = new(() => new()); - - private BufferWriter Buffer => _threadBuffer.Value!; public LinuxUtilizationParserCgroupV1(IFileSystem fileSystem, IUserHz userHz) { @@ -98,9 +96,10 @@ public LinuxUtilizationParserCgroupV1(IFileSystem fileSystem, IUserHz userHz) public long GetCgroupCpuUsageInNanoseconds() { - _fileSystem.ReadAll(_cpuacctUsage, Buffer); + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + _fileSystem.ReadAll(_cpuacctUsage, bufferWriter.Buffer); - var usage = Buffer.WrittenSpan; + var usage = bufferWriter.Buffer.WrittenSpan; _ = GetNextNumber(usage, out var nanoseconds); @@ -109,8 +108,6 @@ public long GetCgroupCpuUsageInNanoseconds() Throw.InvalidOperationException($"Could not get cpu usage from '{_cpuacctUsage}'. Expected positive number, but got '{new string(usage)}'."); } - Buffer.Reset(); - return nanoseconds; } @@ -120,14 +117,15 @@ public long GetHostCpuUsageInNanoseconds() const int NumberOfColumnsRepresentingCpuUsage = 8; const int NanosecondsInSecond = 1_000_000_000; - _fileSystem.ReadFirstLine(_procStat, Buffer); + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + _fileSystem.ReadFirstLine(_procStat, bufferWriter.Buffer); - var stat = Buffer.WrittenSpan; + var stat = bufferWriter.Buffer.WrittenSpan; var total = 0L; - if (!Buffer.WrittenSpan.StartsWith(StartingTokens)) + if (!bufferWriter.Buffer.WrittenSpan.StartsWith(StartingTokens)) { - Throw.InvalidOperationException($"Expected proc/stat to start with '{StartingTokens}' but it was '{new string(Buffer.WrittenSpan)}'."); + Throw.InvalidOperationException($"Expected proc/stat to start with '{StartingTokens}' but it was '{new string(bufferWriter.Buffer.WrittenSpan)}'."); } stat = stat.Slice(StartingTokens.Length, stat.Length - StartingTokens.Length); @@ -150,8 +148,6 @@ public long GetHostCpuUsageInNanoseconds() stat = stat.Slice(next, stat.Length - next); } - Buffer.Reset(); - return (long)(total / (double)_userHz * NanosecondsInSecond); } @@ -183,19 +179,22 @@ public float GetCgroupRequestCpu() public ulong GetAvailableMemoryInBytes() { const long UnsetCgroupMemoryLimit = 9_223_372_036_854_771_712; + long maybeMemory = UnsetCgroupMemoryLimit; - _fileSystem.ReadAll(_memoryLimitInBytes, Buffer); + // Constrain the scope of the buffer because GetHostAvailableMemory is allocating its own buffer. + using (ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool)) + { + _fileSystem.ReadAll(_memoryLimitInBytes, bufferWriter.Buffer); - var memoryBuffer = Buffer.WrittenSpan; - _ = GetNextNumber(memoryBuffer, out var maybeMemory); + var memoryBuffer = bufferWriter.Buffer.WrittenSpan; + _ = GetNextNumber(memoryBuffer, out maybeMemory); - if (maybeMemory == -1) - { - Throw.InvalidOperationException($"Could not parse '{_memoryLimitInBytes}' content. Expected to find available memory in bytes but got '{new string(memoryBuffer)}' instead."); + if (maybeMemory == -1) + { + Throw.InvalidOperationException($"Could not parse '{_memoryLimitInBytes}' content. Expected to find available memory in bytes but got '{new string(memoryBuffer)}' instead."); + } } - Buffer.Reset(); - return maybeMemory == UnsetCgroupMemoryLimit ? GetHostAvailableMemory() : (ulong)maybeMemory; @@ -205,8 +204,9 @@ public ulong GetMemoryUsageInBytes() { const string TotalInactiveFile = "total_inactive_file"; - _fileSystem.ReadAll(_memoryStat, Buffer); - var memoryFile = Buffer.WrittenSpan; + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + _fileSystem.ReadAll(_memoryStat, bufferWriter.Buffer); + var memoryFile = bufferWriter.Buffer.WrittenSpan; var index = memoryFile.IndexOf(TotalInactiveFile.AsSpan()); @@ -223,11 +223,11 @@ public ulong GetMemoryUsageInBytes() Throw.InvalidOperationException($"The value of total_inactive_file found in '{_memoryStat}' is not a positive number: '{new string(inactiveMemorySlice)}'."); } - Buffer.Reset(); + bufferWriter.Buffer.Reset(); - _fileSystem.ReadAll(_memoryUsageInBytes, Buffer); + _fileSystem.ReadAll(_memoryUsageInBytes, bufferWriter.Buffer); - var containerMemoryUsageFile = Buffer.WrittenSpan; + var containerMemoryUsageFile = bufferWriter.Buffer.WrittenSpan; var next = GetNextNumber(containerMemoryUsageFile, out var containerMemoryUsage); // this file format doesn't expect to contain anything after the number. @@ -237,7 +237,7 @@ public ulong GetMemoryUsageInBytes() $"We tried to read '{_memoryUsageInBytes}', and we expected to get a positive number but instead it was: '{new string(containerMemoryUsageFile)}'."); } - Buffer.Reset(); + bufferWriter.Buffer.Reset(); var memoryUsage = containerMemoryUsage - inactiveMemory; @@ -256,8 +256,9 @@ public ulong GetHostAvailableMemory() // The value we are interested in starts with this. We just want to make sure it is true. const string MemTotal = "MemTotal:"; - _fileSystem.ReadFirstLine(_memInfo, Buffer); - var firstLine = Buffer.WrittenSpan; + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + _fileSystem.ReadFirstLine(_memInfo, bufferWriter.Buffer); + var firstLine = bufferWriter.Buffer.WrittenSpan; if (!firstLine.StartsWith(MemTotal)) { @@ -291,8 +292,6 @@ public ulong GetHostAvailableMemory() $"We tried to convert total memory usage value from '{_memInfo}' to bytes, but we've got a unit that we don't recognize: '{new string(unit)}'.") }; - Buffer.Reset(); - return u; } @@ -302,8 +301,9 @@ public ulong GetHostAvailableMemory() /// public float GetHostCpuCount() { - _fileSystem.ReadFirstLine(_cpuSetCpus, Buffer); - var stats = Buffer.WrittenSpan; + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + _fileSystem.ReadFirstLine(_cpuSetCpus, bufferWriter.Buffer); + var stats = bufferWriter.Buffer.WrittenSpan; if (stats.IsEmpty) { @@ -358,8 +358,6 @@ public float GetHostCpuCount() stats = stats.Slice(groupIndex + 1); } - Buffer.Reset(); - return cpuCount; static void ThrowException(ReadOnlySpan content) => @@ -403,13 +401,13 @@ private static int GetNextNumber(ReadOnlySpan buffer, out long number) private bool TryGetCpuUnitsFromCgroups(IFileSystem fileSystem, out float cpuUnits) { - fileSystem.ReadFirstLine(_cpuCfsQuotaUs, Buffer); + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + fileSystem.ReadFirstLine(_cpuCfsQuotaUs, bufferWriter.Buffer); - var quotaBuffer = Buffer.WrittenSpan; + var quotaBuffer = bufferWriter.Buffer.WrittenSpan; if (quotaBuffer.IsEmpty || (quotaBuffer.Length == 2 && quotaBuffer[0] == '-' && quotaBuffer[1] == '1')) { - Buffer.Reset(); cpuUnits = -1; return false; } @@ -421,14 +419,13 @@ private bool TryGetCpuUnitsFromCgroups(IFileSystem fileSystem, out float cpuUnit Throw.InvalidOperationException($"Could not parse '{_cpuCfsQuotaUs}'. Expected an integer but got: '{new string(quotaBuffer)}'."); } - Buffer.Reset(); + bufferWriter.Buffer.Reset(); - fileSystem.ReadFirstLine(_cpuCfsPeriodUs, Buffer); - var periodBuffer = Buffer.WrittenSpan; + fileSystem.ReadFirstLine(_cpuCfsPeriodUs, bufferWriter.Buffer); + var periodBuffer = bufferWriter.Buffer.WrittenSpan; if (periodBuffer.IsEmpty || (periodBuffer.Length == 2 && periodBuffer[0] == '-' && periodBuffer[1] == '1')) { - Buffer.Reset(); cpuUnits = -1; return false; } @@ -440,8 +437,6 @@ private bool TryGetCpuUnitsFromCgroups(IFileSystem fileSystem, out float cpuUnit Throw.InvalidOperationException($"Could not parse '{_cpuCfsPeriodUs}'. Expected to get an integer but got: '{new string(periodBuffer)}'."); } - Buffer.Reset(); - cpuUnits = (float)quota / period; return true; } @@ -462,8 +457,9 @@ private bool TryGetCgroupRequestCpu(IFileSystem fileSystem, out float cpuUnits) return false; } - fileSystem.ReadFirstLine(_cpuPodWeight, Buffer); - var cpuPodWeightBuffer = Buffer.WrittenSpan; + using ReturnableBufferWriter bufferWriter = new(_sharedBufferWriterPool); + fileSystem.ReadFirstLine(_cpuPodWeight, bufferWriter.Buffer); + var cpuPodWeightBuffer = bufferWriter.Buffer.WrittenSpan; _ = GetNextNumber(cpuPodWeightBuffer, out var cpuPodWeight); if (cpuPodWeightBuffer.IsEmpty || (cpuPodWeightBuffer.Length == 2 && cpuPodWeightBuffer[0] == '-' && cpuPodWeightBuffer[1] == '1')) @@ -471,7 +467,6 @@ private bool TryGetCgroupRequestCpu(IFileSystem fileSystem, out float cpuUnits) Throw.InvalidOperationException($"Could not parse '{_cpuPodWeight}' content. Expected to find CPU weight but got '{new string(cpuPodWeightBuffer)}' instead."); } - Buffer.Reset(); var result = cpuPodWeight / CpuShares; cpuUnits = result; return true; diff --git a/src/Shared/BufferWriterPool/ReturnableBufferWriter.cs b/src/Shared/BufferWriterPool/ReturnableBufferWriter.cs new file mode 100644 index 00000000000..4675f84349e --- /dev/null +++ b/src/Shared/BufferWriterPool/ReturnableBufferWriter.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using Microsoft.Extensions.ObjectPool; + +#pragma warning disable CA1716 +namespace Microsoft.Shared.Pools; +#pragma warning restore CA1716 + +/// +/// Represents a buffer writer that can be automatically returned to an object pool upon dispose. +/// +/// The type of the elements in the buffer. +#if !SHARED_PROJECT +[System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] +#endif +internal readonly struct ReturnableBufferWriter : IDisposable +{ + private readonly ObjectPool> _pool; + + /// + /// Initializes a new instance of the struct. + /// + /// The object pool to return the buffer writer to. + public ReturnableBufferWriter(ObjectPool> pool) + { + _pool = pool; + Buffer = pool.Get(); + } + + /// + /// Gets the buffer writer. + /// + public BufferWriter Buffer { get; } + + /// + /// Disposes the buffer writer and returns it to the object pool. + /// + public readonly void Dispose() + { + Buffer.Reset(); + _pool.Return(Buffer); + } +}