avformat/http: stale Retry-After value leaks into later unrelated reconnects and can abort or arbitrarily delay them

[ronag]

Summary

HTTPContext.retry_after is set whenever any HTTP response carries a Retry-After header, but it is only cleared in the one place that consumes it: the failure-retry branch of http_open_cnx(). If a response carrying Retry-After does not go through that branch — e.g. a 3xx redirect with Retry-After (RFC-legitimate per RFC 9110 §10.2.3), a 401 retried via the auth path, or any 2xx that includes the header — the value silently persists in the context for the lifetime of the stream (and across ff_http_do_new_request() reuse, which resets other per-request state but not this field). When a completely unrelated transient failure later triggers the reconnect path, the stale value is consumed: if it exceeds reconnect_delay_max (default 120 s) the reconnect is instantly aborted with goto fail; otherwise it forces an arbitrary sleep that has nothing to do with the current failure. Stale state from one response thus changes the error/retry behavior of a completely different request.

Location

• Consumption (only place the value is reset): http_open_cnx() —

  FFmpeg/libavformat/http.c

  Lines 471 to 478 in 9a83bff

  	/* Both fields here are in seconds. */
  	if (s->respect_retry_after && s->retry_after > 0) {
  	reconnect_delay = s->retry_after;
  	if (reconnect_delay > s->reconnect_delay_max)
  	goto fail;
  	s->retry_after = 0;
  	s->nb_retries++;
  	}

• Set on every response that carries the header: process_line() —

  FFmpeg/libavformat/http.c

  Lines 1358 to 1369 in 9a83bff

  	} else if (!av_strcasecmp(tag, "Retry-After")) {
  	/* The header can be either an integer that represents seconds, or a date. */
  	struct tm tm;
  	int date_ret = parse_http_date(p, &tm);
  	if (!date_ret) {
  	time_t retry = av_timegm(&tm);
  	int64_t now = av_gettime() / 1000000;
  	int64_t diff = ((int64_t) retry) - now;
  	s->retry_after = (unsigned int) FFMAX(0, diff);
  	} else {
  	s->retry_after = strtoul(p, NULL, 10);
  	}

• Missing reset in the per-response init block of http_read_header() —

  FFmpeg/libavformat/http.c

  Lines 1480 to 1484 in 9a83bff

  	av_freep(&s->new_location);
  	s->expires = 0;
  	s->chunksize = UINT64_MAX;
  	s->filesize_from_content_range = UINT64_MAX;

Details

process_line() stores the header value unconditionally for any response:

} else if (!av_strcasecmp(tag, "Retry-After")) {
    /* The header can be either an integer that represents seconds, or a date. */
    struct tm tm;
    int date_ret = parse_http_date(p, &tm);
    if (!date_ret) {
        time_t retry   = av_timegm(&tm);
        int64_t now    = av_gettime() / 1000000;
        int64_t diff   = ((int64_t) retry) - now;
        s->retry_after = (unsigned int) FFMAX(0, diff);
    } else {
        s->retry_after = strtoul(p, NULL, 10);
    }
}

The only consumer is http_open_cnx()'s failure-retry branch (lines 464–495), which also performs the only reset:

    ret = http_open_cnx_internal(h, options);
    if (ret < 0) {
        if (!http_should_reconnect(s, ret) ||
            reconnect_delay > s->reconnect_delay_max ||
            (s->reconnect_max_retries >= 0 && conn_attempts > s->reconnect_max_retries) ||
            reconnect_delay_total > s->reconnect_delay_total_max)
            goto fail;

        /* Both fields here are in seconds. */
        if (s->respect_retry_after && s->retry_after > 0) {
            reconnect_delay = s->retry_after;
            if (reconnect_delay > s->reconnect_delay_max)
                goto fail;
            s->retry_after = 0;
            s->nb_retries++;
        }

Meanwhile http_read_header() resets other per-response state but not retry_after (lines 1480–1484):

    av_freep(&s->new_location);
    s->expires = 0;
    s->chunksize = UINT64_MAX;
    s->filesize_from_content_range = UINT64_MAX;

A grep -n retry_after libavformat/http.c confirms there is no other reset: the field is set at lines 1366/1368 and only consumed/cleared at lines 472–476. Responses handled by the redirect branch (lines 514–540) and the 401/407 auth-retry branches (lines 498–513) reach neither the consumption nor any reset. ff_http_do_new_request2() (lines 597–600) likewise resets end_chunked_post, chunkend, off, icy_data_read — but not retry_after.

Step-by-step trace:

1. Open an http(s) URL with e.g. -reconnect_on_http_error 5xx (or reconnect_on_network_error, or this fork's reconnect_partial 416 polling). The server answers 302 with Location: plus Retry-After: 600 (legitimate per RFC 9110 — Retry-After is also defined for 3xx to mean "minimum wait before following the redirect").
2. http_open_cnx(): the redirect branch runs (no consumption of retry_after), goto redo, and the new location returns 200. The function returns 0 with s->retry_after == 600 still set.
3. Hours later, mid-stream, a reconnect is needed: http_buf_read() returns EAGAIN and http_read_stream() (line 1922) calls http_open_cnx() directly, or a network error drives http_read_stream()'s retry loop through http_seek_internal(..., 1) → http_open_cnx(). The reopen fails transiently (e.g. a 503).
4. In http_open_cnx(): ret < 0, http_should_reconnect() matches 5xx, so a retry is intended. Line 472 finds the stale value: s->respect_retry_after (default 1) and s->retry_after == 600 → reconnect_delay = 600 → line 474: 600 > s->reconnect_delay_max (default 120) → goto fail. The reconnect the user explicitly configured is bypassed and a hard error is returned, solely because of a Retry-After received on a long-gone redirect response.
5. If the stale value were instead e.g. 60, the first retry sleeps 60 s for no current reason, and the exponential backoff at line 486 then escalates from there (1 + 2*60 = 121 > 120 → give up on the next attempt) instead of starting from 0.

Impact

• User-configured reconnect behavior (reconnect_on_http_error, reconnect_on_network_error, reconnect_partial) can be silently disabled: a single stale Retry-After > reconnect_delay_max turns the very first retry attempt into an immediate hard failure, killing long-running streams on the first transient hiccup.
• Smaller stale values inject arbitrary multi-second/minute sleeps into reconnects of unrelated requests and poison the backoff escalation, drastically reducing the effective retry budget.
• A server (or any intermediary able to inject a header on one response, e.g. a redirector or captive portal) can durably degrade or break the client's reconnect behavior for the rest of the connection's lifetime.

Severity: medium (reliability/availability; no memory safety implications).

This bug also exists in upstream FFmpeg (upstream/master, verified via git show upstream/master:libavformat/http.c): the same set-at-process_line (lines 1325/1327) / consume-only-in-http_open_cnx (lines 444–448) / no-reset-in-http_read_header pattern is present. It is worth reporting upstream as well.

Suggested fix

Clear the per-response value at the start of each header parse, next to the other per-response resets in http_read_header():

--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -1478,10 +1478,11 @@ static int http_read_header(URLContext *h)
     char line[MAX_URL_SIZE];
     int err = 0, http_err = 0;
 
     av_freep(&s->new_location);
     s->expires = 0;
     s->chunksize = UINT64_MAX;
     s->filesize_from_content_range = UINT64_MAX;
+    s->retry_after = 0;
 
     for (;;) {
         int parsed_http_code = 0;

This preserves the intended behavior — a Retry-After applies to the immediate retry of the request whose response carried it (consumed once at lines 472–476, where parsing happens before the failure branch loops back) — while preventing the value from leaking into later, unrelated requests and reconnects. The same one-line fix applies to upstream's http_read_header() init block.