From c1241142cd9fb39e7cde812d788d6418609ff6c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Thu, 28 Dec 2017 08:44:42 +0100 Subject: [PATCH 1/8] Stack.Pop RCE With value types the effect is not so big, because there is still one (manual) check for bounds. For reference types one bounds-check can be saved, so there is a win. --- .../src/System/Collections/Generic/Stack.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index e84e35b366c7..8b16d6f344e5 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -225,16 +225,22 @@ public bool TryPeek(out T result) // throws an InvalidOperationException. public T Pop() { - if (_size == 0) + int size = _size - 1; + T[] array = _array; + + // if (_size == 0) is equivalent to if (size == -1), and this case + // is covered with (uint)size, thus allowing RCE https://github.com/dotnet/coreclr/pull/9773 + if ((uint)size >= (uint)array.Length) { ThrowForEmptyStack(); } _version++; - T item = _array[--_size]; + _size = size; + T item = array[size]; if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - _array[_size] = default(T); // Free memory quicker. + array[size] = default; // Free memory quicker. } return item; } From 012333094dd2b2052eaf6daebaaba2a98f25d2b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Thu, 28 Dec 2017 11:06:32 +0100 Subject: [PATCH 2/8] Applied same optimizations as for Pop on Peek, TryPeek, TryPop, Push --- .../src/System/Collections/Generic/Stack.cs | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index 8b16d6f344e5..47e608111f0b 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -202,22 +202,28 @@ public void TrimExcess() // is empty, Peek throws an InvalidOperationException. public T Peek() { - if (_size == 0) + int size = _size - 1; + T[] array = _array; + + if ((uint)size >= (uint)array.Length) { ThrowForEmptyStack(); } - return _array[_size - 1]; + return array[size]; } public bool TryPeek(out T result) { - if (_size == 0) + int size = _size - 1; + T[] array = _array; + + if ((uint)size >= (uint)array.Length) { - result = default(T); + result = default; return false; } - result = _array[_size - 1]; + result = array[size]; return true; } @@ -247,17 +253,21 @@ public T Pop() public bool TryPop(out T result) { - if (_size == 0) + int size = _size - 1; + T[] array = _array; + + if ((uint)size >= (uint)array.Length) { - result = default(T); + result = default; return false; } _version++; - result = _array[--_size]; + _size = size; + result = array[size]; if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - _array[_size] = default(T); // Free memory quicker. + array[size] = default; // Free memory quicker. } return true; } @@ -265,12 +275,17 @@ public bool TryPop(out T result) // Pushes an item to the top of the stack. public void Push(T item) { - if (_size == _array.Length) + int size = _size; + T[] array = _array; + + if ((uint)size >= (uint)array.Length) { - Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length); + Array.Resize(ref array, (array.Length == 0) ? DefaultCapacity : 2 * array.Length); + _array = array; } - _array[_size++] = item; + array[size++] = item; _version++; + _size = size; } // Copies the Stack to an array, in the same order Pop would return the items. From e7308d8b0685078f754361a2b7c6ad2cabaf424a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Thu, 28 Dec 2017 14:20:49 +0100 Subject: [PATCH 3/8] Revert change for Push I can't see a real win, sometimes it's faster and sometimes slower. On Linux the tendency is to be slower. Therefore the state as is will be remained. --- .../src/System/Collections/Generic/Stack.cs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index 47e608111f0b..ccfffe8e2015 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -275,17 +275,12 @@ public bool TryPop(out T result) // Pushes an item to the top of the stack. public void Push(T item) { - int size = _size; - T[] array = _array; - - if ((uint)size >= (uint)array.Length) + if (_size == _array.Length) { - Array.Resize(ref array, (array.Length == 0) ? DefaultCapacity : 2 * array.Length); - _array = array; + Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length); } - array[size++] = item; + _array[_size++] = item; _version++; - _size = size; } // Copies the Stack to an array, in the same order Pop would return the items. From d2f5b2c9094c6c0ac9a9fb67298c99103a01b204 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Thu, 28 Dec 2017 20:16:10 +0100 Subject: [PATCH 4/8] Stack.Push with hot-/cold-path (PushWithResize) --- .../src/System/Collections/Generic/Stack.cs | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index ccfffe8e2015..7e0624d76a7f 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -275,12 +275,29 @@ public bool TryPop(out T result) // Pushes an item to the top of the stack. public void Push(T item) { - if (_size == _array.Length) + int size = _size; + T[] array = _array; + + if ((uint)size < (uint)array.Length) { - Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length); + array[size] = item; + _version++; + _size++; } - _array[_size++] = item; + else + { + PushWithResize(item); + } + } + + // Non-inline from Stack.Push to improve its code quality as uncommon path + [MethodImpl(MethodImplOptions.NoInlining)] + private void PushWithResize(T item) + { + Array.Resize(ref _array, (_array.Length == 0) ? DefaultCapacity : 2 * _array.Length); + _array[_size] = item; _version++; + _size++; } // Copies the Stack to an array, in the same order Pop would return the items. From f337882e119892d9ababbb8984ec7f66793a8ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Thu, 28 Dec 2017 20:26:27 +0100 Subject: [PATCH 5/8] Addressed PR feedback Cf. https://github.com/dotnet/corefx/pull/26086#discussion_r158987900 --- src/System.Collections/src/System/Collections/Generic/Stack.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index 7e0624d76a7f..a99ec901b635 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -235,7 +235,8 @@ public T Pop() T[] array = _array; // if (_size == 0) is equivalent to if (size == -1), and this case - // is covered with (uint)size, thus allowing RCE https://github.com/dotnet/coreclr/pull/9773 + // is covered with (uint)size, thus allowing bounds check elimination + // https://github.com/dotnet/coreclr/pull/9773 if ((uint)size >= (uint)array.Length) { ThrowForEmptyStack(); From b0bfd83db525f4ce2d095a7a631f468dd90789ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 29 Dec 2017 17:17:02 +0100 Subject: [PATCH 6/8] Array-copy in Peek is not necessary, JIT can do the same As of PR-feedback https://github.com/dotnet/corefx/pull/26086#discussion_r159071441 --- .../src/System/Collections/Generic/Stack.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index a99ec901b635..4f7bf20a7219 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -203,14 +203,13 @@ public void TrimExcess() public T Peek() { int size = _size - 1; - T[] array = _array; - if ((uint)size >= (uint)array.Length) + if ((uint)size >= (uint)_array.Length) { ThrowForEmptyStack(); } - return array[size]; + return _array[size]; } public bool TryPeek(out T result) From 0a2e11efb115780bef344ddd2e5d1aa9e9eda38f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Sat, 13 Jan 2018 14:39:01 +0100 Subject: [PATCH 7/8] Reverted b0bfd83 Cf. https://github.com/dotnet/corefx/pull/26086#issuecomment-357314683 --- .../src/System/Collections/Generic/Stack.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index 4f7bf20a7219..a99ec901b635 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -203,13 +203,14 @@ public void TrimExcess() public T Peek() { int size = _size - 1; + T[] array = _array; - if ((uint)size >= (uint)_array.Length) + if ((uint)size >= (uint)array.Length) { ThrowForEmptyStack(); } - return _array[size]; + return array[size]; } public bool TryPeek(out T result) From 084acb7e6e5c18e4ca7342dc40a97396264f0613 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Mon, 29 Jan 2018 18:22:02 +0100 Subject: [PATCH 8/8] Addressed PR feedback Cf. https://github.com/dotnet/corefx/pull/26086#discussion_r164453817 --- src/System.Collections/src/System/Collections/Generic/Stack.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Collections/src/System/Collections/Generic/Stack.cs b/src/System.Collections/src/System/Collections/Generic/Stack.cs index a99ec901b635..e474794e94db 100644 --- a/src/System.Collections/src/System/Collections/Generic/Stack.cs +++ b/src/System.Collections/src/System/Collections/Generic/Stack.cs @@ -283,7 +283,7 @@ public void Push(T item) { array[size] = item; _version++; - _size++; + _size = size + 1; } else {