Harden ArrayBuilder

Since we're using the ArrayPool, it's really essential that we prevent
use-after-free bugs. I'm currently tracking one down.
This commit is contained in:
Ryan Nowak 2019-08-10 19:35:13 -07:00
parent 89bf58445f
commit ab006e10b0
2 changed files with 34 additions and 0 deletions

View File

@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved. // Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
using System;
using System.Buffers; using System.Buffers;
using System.Linq; using System.Linq;
using Xunit; using Xunit;
@ -261,6 +262,7 @@ namespace Microsoft.AspNetCore.Components.RenderTree
Assert.Single(ArrayPool.ReturnedBuffers); Assert.Single(ArrayPool.ReturnedBuffers);
var returnedBuffer = Assert.Single(ArrayPool.ReturnedBuffers); var returnedBuffer = Assert.Single(ArrayPool.ReturnedBuffers);
Assert.Same(buffer, returnedBuffer); Assert.Same(buffer, returnedBuffer);
Assert.NotSame(builder.Buffer, buffer); // Prevents use after free
} }
[Fact] [Fact]
@ -281,6 +283,21 @@ namespace Microsoft.AspNetCore.Components.RenderTree
Assert.Same(buffer, returnedBuffer); Assert.Same(buffer, returnedBuffer);
} }
[Fact]
public void Dispose_ThrowsOnReuse()
{
// Arrange
var builder = CreateArrayBuilder();
builder.Append(1);
var buffer = builder.Buffer;
builder.Dispose();
Assert.Single(ArrayPool.ReturnedBuffers);
// Act & Assert
Assert.Throws<ObjectDisposedException>(() => builder.Append(1));
}
[Fact] [Fact]
public void UnusedBufferIsReturned_OnResize() public void UnusedBufferIsReturned_OnResize()
{ {

View File

@ -160,6 +160,16 @@ namespace Microsoft.AspNetCore.Components.RenderTree
private void GrowBuffer(int desiredCapacity) private void GrowBuffer(int desiredCapacity)
{ {
// When we dispose, we set the count back to zero and return the array.
//
// If someone tries to do something that would require non-zero storage then
// this is a use-after-free. Throwing here is an easy way to prevent that without
// introducing overhead to every method.
if (_disposed)
{
ThrowObjectDisposedException();
}
var newCapacity = Math.Max(desiredCapacity, _minCapacity); var newCapacity = Math.Max(desiredCapacity, _minCapacity);
Debug.Assert(newCapacity > _items.Length); Debug.Assert(newCapacity > _items.Length);
@ -188,6 +198,8 @@ namespace Microsoft.AspNetCore.Components.RenderTree
{ {
_disposed = true; _disposed = true;
ReturnBuffer(); ReturnBuffer();
_items = Empty;
_itemsInUse = 0;
} }
} }
@ -195,5 +207,10 @@ namespace Microsoft.AspNetCore.Components.RenderTree
{ {
throw new ArgumentOutOfRangeException("index"); throw new ArgumentOutOfRangeException("index");
} }
private static void ThrowObjectDisposedException()
{
throw new ObjectDisposedException(objectName: null);
}
} }
} }