Do not reorder HTTP header values (dotnet#65249)

* Do not reorder HTTP header values * fix complie error * make the changes @ danmoseley recommended * make the changes @MihaZupan recommended * make the changes @MihaZupan recommended * make the changes @MihaZupan recommended * make the changes @MihaZupan recommended * make the changes @MihaZupan recommended * check array length * fix unit test * Update src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * make the changes @MihaZupan recommended * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * chang unit test * GetParsedValueLength * fix build error * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * unit test * make changes @geoffkizer recommended * CloneAndAddValue for InvalidValues * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> * Final nits Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
radekdoulik · Mar 30, 2022 · c749ffc · c749ffc
1 parent 98719ba
commit c749ffc
Show file tree

Hide file tree

Showing 13 changed files with 346 additions and 191 deletions.
diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs
@@ -1,6 +1,7 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Generic;
 using System.Diagnostics;
 
 namespace System.Net.Http.Headers
@@ -22,8 +23,29 @@ private CacheControlHeaderParser()
         protected override int GetParsedValueLength(string value, int startIndex, object? storeValue,
             out object? parsedValue)
         {
-            CacheControlHeaderValue? temp = storeValue as CacheControlHeaderValue;
-            Debug.Assert(storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue");
+            CacheControlHeaderValue? temp = null;
+            bool isInvalidValue = true;
+            if (storeValue is List<object> list)
+            {
+                foreach (object item in list)
+                {
+                    if (item is not HttpHeaders.InvalidValue)
+                    {
+                        isInvalidValue = false;
+                        temp = item as CacheControlHeaderValue;
+                        break;
+                    }
+                }
+            }
+            else
+            {
+                if (storeValue is not HttpHeaders.InvalidValue)
+                {
+                    isInvalidValue = false;
+                    temp = storeValue as CacheControlHeaderValue;
+                }
+            }
+            Debug.Assert(isInvalidValue || storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue");
 
             int resultLength = CacheControlHeaderValue.GetCacheControlLength(value, startIndex, temp, out temp);
 

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs
@@ -287,7 +287,7 @@ internal static int GetNextNonEmptyOrWhitespaceIndex(string input, int startInde
         {
             Debug.Assert(store != null);
 
-            object? storedValue = store.GetParsedValues(descriptor);
+            object? storedValue = store.GetSingleParsedValue(descriptor);
             if (storedValue != null)
             {
                 return (DateTimeOffset)storedValue;
@@ -304,7 +304,7 @@ internal static int GetNextNonEmptyOrWhitespaceIndex(string input, int startInde
         {
             Debug.Assert(store != null);
 
-            object? storedValue = store.GetParsedValues(descriptor);
+            object? storedValue = store.GetSingleParsedValue(descriptor);
             if (storedValue != null)
             {
                 return (TimeSpan)storedValue;

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs
@@ -19,7 +19,7 @@ public sealed class HttpContentHeaders : HttpHeaders
 
         public ContentDispositionHeaderValue? ContentDisposition
         {
-            get { return (ContentDispositionHeaderValue?)GetParsedValues(KnownHeaders.ContentDisposition.Descriptor); }
+            get { return (ContentDispositionHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentDisposition.Descriptor); }
             set { SetOrRemoveParsedValue(KnownHeaders.ContentDisposition.Descriptor, value); }
         }
 
@@ -36,7 +36,7 @@ public long? ContentLength
             get
             {
                 // 'Content-Length' can only hold one value. So either we get 'null' back or a boxed long value.
-                object? storedValue = GetParsedValues(KnownHeaders.ContentLength.Descriptor);
+                object? storedValue = GetSingleParsedValue(KnownHeaders.ContentLength.Descriptor);
 
                 // Only try to calculate the length if the user didn't set the value explicitly using the setter.
                 if (!_contentLengthSet && (storedValue == null))
@@ -72,25 +72,25 @@ public long? ContentLength
 
         public Uri? ContentLocation
         {
-            get { return (Uri?)GetParsedValues(KnownHeaders.ContentLocation.Descriptor); }
+            get { return (Uri?)GetSingleParsedValue(KnownHeaders.ContentLocation.Descriptor); }
             set { SetOrRemoveParsedValue(KnownHeaders.ContentLocation.Descriptor, value); }
         }
 
         public byte[]? ContentMD5
         {
-            get { return (byte[]?)GetParsedValues(KnownHeaders.ContentMD5.Descriptor); }
+            get { return (byte[]?)GetSingleParsedValue(KnownHeaders.ContentMD5.Descriptor); }
             set { SetOrRemoveParsedValue(KnownHeaders.ContentMD5.Descriptor, value); }
         }
 
         public ContentRangeHeaderValue? ContentRange
         {
-            get { return (ContentRangeHeaderValue?)GetParsedValues(KnownHeaders.ContentRange.Descriptor); }
+            get { return (ContentRangeHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentRange.Descriptor); }
             set { SetOrRemoveParsedValue(KnownHeaders.ContentRange.Descriptor, value); }
         }
 
         public MediaTypeHeaderValue? ContentType
         {
-            get { return (MediaTypeHeaderValue?)GetParsedValues(KnownHeaders.ContentType.Descriptor); }
+            get { return (MediaTypeHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentType.Descriptor); }
             set { SetOrRemoveParsedValue(KnownHeaders.ContentType.Descriptor, value); }
         }
 

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs
@@ -22,7 +22,7 @@ internal sealed class HttpGeneralHeaders
 
         public CacheControlHeaderValue? CacheControl
         {
-            get { return (CacheControlHeaderValue?)_parent.GetParsedValues(KnownHeaders.CacheControl.Descriptor); }
+            get { return (CacheControlHeaderValue?)_parent.GetSingleParsedValue(KnownHeaders.CacheControl.Descriptor); }
             set { _parent.SetOrRemoveParsedValue(KnownHeaders.CacheControl.Descriptor, value); }
         }
 

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs
@@ -86,7 +86,7 @@ public void CopyTo(T[] array!!, int arrayIndex)
                 throw new ArgumentOutOfRangeException(nameof(arrayIndex));
             }
 
-            object? storeValue = _store.GetParsedValues(_descriptor);
+            object? storeValue = _store.GetParsedAndInvalidValues(_descriptor);
 
             if (storeValue == null)
             {
@@ -97,18 +97,30 @@ public void CopyTo(T[] array!!, int arrayIndex)
 
             if (storeValues == null)
             {
-                // We only have 1 value: If it is the "special value" just return, otherwise add the value to the
-                // array and return.
-                Debug.Assert(storeValue is T);
-                if (arrayIndex == array.Length)
+                if (storeValue is not HttpHeaders.InvalidValue)
                 {
-                    throw new ArgumentException(SR.net_http_copyto_array_too_small);
+                    Debug.Assert(storeValue is T);
+                    if (arrayIndex == array.Length)
+                    {
+                        throw new ArgumentException(SR.net_http_copyto_array_too_small);
+                    }
+                    array[arrayIndex] = (T)storeValue;
                 }
-                array[arrayIndex] = (T)storeValue;
             }
             else
             {
-                storeValues.CopyTo(array, arrayIndex);
+                foreach (object item in storeValues)
+                {
+                    if (item is not HttpHeaders.InvalidValue)
+                    {
+                        Debug.Assert(item is T);
+                        if (arrayIndex == array.Length)
+                        {
+                            throw new ArgumentException(SR.net_http_copyto_array_too_small);
+                        }
+                        array[arrayIndex++] = (T)item;
+                    }
+                }
             }
         }
 
@@ -122,8 +134,8 @@ public bool Remove(T item)
 
         public IEnumerator<T> GetEnumerator()
         {
-            object? storeValue = _store.GetParsedValues(_descriptor);
-            return storeValue is null ?
+            object? storeValue = _store.GetParsedAndInvalidValues(_descriptor);
+            return storeValue is null || storeValue is HttpHeaders.InvalidValue ?
                 ((IEnumerable<T>)Array.Empty<T>()).GetEnumerator() : // use singleton empty array enumerator
                 Iterate(storeValue);
 
@@ -134,6 +146,10 @@ static IEnumerator<T> Iterate(object storeValue)
                     // We have multiple values. Iterate through the values and return them.
                     foreach (object item in storeValues)
                     {
+                        if (item is HttpHeaders.InvalidValue)
+                        {
+                            continue;
+                        }
                         Debug.Assert(item is T);
                         yield return (T)item;
                     }
@@ -178,7 +194,7 @@ private int GetCount()
         {
             // This is an O(n) operation.
 
-            object? storeValue = _store.GetParsedValues(_descriptor);
+            object? storeValue = _store.GetParsedAndInvalidValues(_descriptor);
 
             if (storeValue == null)
             {
@@ -189,11 +205,23 @@ private int GetCount()
 
             if (storeValues == null)
             {
-                return 1;
+                if (storeValue is not HttpHeaders.InvalidValue)
+                {
+                    return 1;
+                }
+                return 0;
             }
             else
             {
-                return storeValues.Count;
+                int count = 0;
+                foreach (object item in storeValues)
+                {
+                    if (item is not HttpHeaders.InvalidValue)
+                    {
+                        count++;
+                    }
+                }
+                return count;
             }
         }
     }