-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Closed
Labels
area-System.Net.Httpbugin-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged
Milestone
Description
Looking at the current Alt-Svc logic, there are other weird things going on:
- It's likely that the current logic for
clearis unreachable- We are reading the list of values via
HttpHeaders.TryGetValueshere. This involves validation, so it goes through theAltSvcHeaderParser, which creates a list ofAltSvcHeaderValues. Those values are thenToStringed and returned fromTryGetValues. Then the loop feeds those strings back toAltSvcHeaderParserand looks at parsed values. AltSvcHeaderValue.ToStringis broken forclearand it will returnclear=":0"; ma=0, which is an invalid value and won't roundtrip through the parser.- As an example, the following code throws on the second
Addvar headers = new HttpResponseMessage().Headers; headers.Add("Alt-Svc", "clear"); string value = headers.GetValues("Alt-Svc").Single(); headers.Clear(); headers.Add("Alt-Svc", value); // The format of value 'clear=":0"; ma=0' is invalid
- Similarly, the second call to the
AltSvcHeaderParserwill fail, and so that loop can never observe theclearvalue
- We are reading the list of values via
- Even if the loop could see
clear, we're still going to use the first authority we saw, even though clear should have been the only value present. (Webreakhere, but maybe we should bereturning?) - We're comparing authorities against each other in a few places with
==and!=, but theHttpAuthorityclass doesn't override these operators, so this is doing reference equality. This is suspicious at best and potentially an error in some places. GetHttp3ConnectionAsynccould end up returningnullhere in a race, leading to a NRE here.
Originally posted by @MihaZupan in #83775 (comment)
Metadata
Metadata
Assignees
Labels
area-System.Net.Httpbugin-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged