Skip to content

Commit

Permalink
fixup! Avoid buffer race conditions in CGroups
Browse files Browse the repository at this point in the history
  • Loading branch information
@RussKie
RussKie committed May 17, 2024
1 parent a5cadfe commit f3a2224
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 48 deletions.
91 changes: 43 additions & 48 deletions ...crosoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -18,6 +18,7 @@ namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux;
internal sealed class LinuxUtilizationParserCgroupV1 : ILinuxUtilizationParser
{
private const float CpuShares = 1024;
private static readonly ObjectPool<BufferWriter<char>> _sharedBufferWriterPool = BufferWriterPool.CreateBufferWriterPool<char>();

/// <remarks>
/// File contains the amount of CPU time (in microseconds) available to the group during each accounting period.
Expand Down Expand Up @@ -86,9 +87,6 @@ internal sealed class LinuxUtilizationParserCgroupV1 : ILinuxUtilizationParser

private readonly IFileSystem _fileSystem;
private readonly long _userHz;
private readonly ThreadLocal<BufferWriter<char>> _threadBuffer = new(() => new());

private BufferWriter<char> Buffer => _threadBuffer.Value!;

public LinuxUtilizationParserCgroupV1(IFileSystem fileSystem, IUserHz userHz)
{
Expand All @@ -98,9 +96,10 @@ public LinuxUtilizationParserCgroupV1(IFileSystem fileSystem, IUserHz userHz)

public long GetCgroupCpuUsageInNanoseconds()
{
_fileSystem.ReadAll(_cpuacctUsage, Buffer);
using ReturnableBufferWriter<char> bufferWriter = new(_sharedBufferWriterPool);
_fileSystem.ReadAll(_cpuacctUsage, bufferWriter.Buffer);

var usage = Buffer.WrittenSpan;
var usage = bufferWriter.Buffer.WrittenSpan;

_ = GetNextNumber(usage, out var nanoseconds);

Expand All @@ -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;
}

Expand All @@ -120,14 +117,15 @@ public long GetHostCpuUsageInNanoseconds()
const int NumberOfColumnsRepresentingCpuUsage = 8;
const int NanosecondsInSecond = 1_000_000_000;

_fileSystem.ReadFirstLine(_procStat, Buffer);
using ReturnableBufferWriter<char> 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);
Expand All @@ -150,8 +148,6 @@ public long GetHostCpuUsageInNanoseconds()
stat = stat.Slice(next, stat.Length - next);
}

Buffer.Reset();

return (long)(total / (double)_userHz * NanosecondsInSecond);
}

Expand Down Expand Up @@ -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<char> 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;
Expand All @@ -205,8 +204,9 @@ public ulong GetMemoryUsageInBytes()
{
const string TotalInactiveFile = "total_inactive_file";

_fileSystem.ReadAll(_memoryStat, Buffer);
var memoryFile = Buffer.WrittenSpan;
using ReturnableBufferWriter<char> bufferWriter = new(_sharedBufferWriterPool);
_fileSystem.ReadAll(_memoryStat, bufferWriter.Buffer);
var memoryFile = bufferWriter.Buffer.WrittenSpan;

var index = memoryFile.IndexOf(TotalInactiveFile.AsSpan());

Expand All @@ -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.
Expand All @@ -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;

Expand All @@ -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<char> bufferWriter = new(_sharedBufferWriterPool);
_fileSystem.ReadFirstLine(_memInfo, bufferWriter.Buffer);
var firstLine = bufferWriter.Buffer.WrittenSpan;

if (!firstLine.StartsWith(MemTotal))
{
Expand Down Expand Up @@ -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;
}

Expand All @@ -302,8 +301,9 @@ public ulong GetHostAvailableMemory()
/// </remarks>
public float GetHostCpuCount()
{
_fileSystem.ReadFirstLine(_cpuSetCpus, Buffer);
var stats = Buffer.WrittenSpan;
using ReturnableBufferWriter<char> bufferWriter = new(_sharedBufferWriterPool);
_fileSystem.ReadFirstLine(_cpuSetCpus, bufferWriter.Buffer);
var stats = bufferWriter.Buffer.WrittenSpan;

if (stats.IsEmpty)
{
Expand Down Expand Up @@ -358,8 +358,6 @@ public float GetHostCpuCount()
stats = stats.Slice(groupIndex + 1);
}

Buffer.Reset();

return cpuCount;

static void ThrowException(ReadOnlySpan<char> content) =>
Expand Down Expand Up @@ -403,13 +401,13 @@ private static int GetNextNumber(ReadOnlySpan<char> buffer, out long number)

private bool TryGetCpuUnitsFromCgroups(IFileSystem fileSystem, out float cpuUnits)
{
fileSystem.ReadFirstLine(_cpuCfsQuotaUs, Buffer);
using ReturnableBufferWriter<char> 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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -462,16 +457,16 @@ private bool TryGetCgroupRequestCpu(IFileSystem fileSystem, out float cpuUnits)
return false;
}

fileSystem.ReadFirstLine(_cpuPodWeight, Buffer);
var cpuPodWeightBuffer = Buffer.WrittenSpan;
using ReturnableBufferWriter<char> 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'))
{
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;
Expand Down
45 changes: 45 additions & 0 deletions src/Shared/BufferWriterPool/ReturnableBufferWriter.cs
View file
Original file line number Diff line number Diff line change
@@ -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

/// <summary>
/// Represents a buffer writer that can be automatically returned to an object pool upon dispose.
/// </summary>
/// <typeparam name="T">The type of the elements in the buffer.</typeparam>
#if !SHARED_PROJECT
[System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
#endif
internal readonly struct ReturnableBufferWriter<T> : IDisposable
{
private readonly ObjectPool<BufferWriter<T>> _pool;

/// <summary>
/// Initializes a new instance of the <see cref="ReturnableBufferWriter{T}"/> struct.
/// </summary>
/// <param name="pool">The object pool to return the buffer writer to.</param>
public ReturnableBufferWriter(ObjectPool<BufferWriter<T>> pool)
{
_pool = pool;
Buffer = pool.Get();
}

/// <summary>
/// Gets the buffer writer.
/// </summary>
public BufferWriter<T> Buffer { get; }

/// <summary>
/// Disposes the buffer writer and returns it to the object pool.
/// </summary>
public readonly void Dispose()
{
Buffer.Reset();
_pool.Return(Buffer);
}
}

0 comments on commit f3a2224

Please sign in to comment.