Skip to content

Parse the HTTP Last-Modified zone as the literal GMT, not %Z#111619

Open
thevar1able wants to merge 3 commits into
masterfrom
fix-http-last-modified-gmt-zone
Open

Parse the HTTP Last-Modified zone as the literal GMT, not %Z#111619
thevar1able wants to merge 3 commits into
masterfrom
fix-http-last-modified-gmt-zone

Conversation

@thevar1able

Copy link
Copy Markdown
Member

Parse the HTTP Last-Modified header's time zone as the literal GMT instead of strptime's %Z.

RFC 7231 IMF-fixdate always ends in the literal GMT, but parseFileInfo in ReadWriteBufferFromHTTP matched it with %Z, which is not portable. musl's %Z only knows the names of the currently configured local time zone, so a zero-length match consumes nothing, GMT is left unparsed, the full-consumption check fails, and the _time virtual column of the url/urlCluster table functions is always NULL. If the time zone database has not been initialized yet, musl's %Z can even dereference a NULL tzname[0]. Matching the literal parses identically on musl (with and without tzset) and glibc.

This is why 04104_url_time_virtual_column failed on every musl configuration; that existing test guards the fix.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed the _time virtual column of the url/urlCluster table functions always being NULL on musl-based builds, caused by non-portable parsing of the HTTP Last-Modified header's time zone.

RFC 7231 IMF-fixdate always ends in the literal "GMT", but the parser
matched it with strptime's %Z. That is not portable: musl's %Z only
knows the names of the currently configured local time zone, so a
zero-length tzname match consumes nothing, "GMT" is left unparsed, the
full-consumption check fails, and the _time virtual column of the url
table function is always NULL (04104_url_time_virtual_column failed on
every musl configuration). If the time zone database has not been
initialized yet, musl's %Z even dereferences a NULL tzname[0].

Match the literal instead; verified to parse identically on musl (with
and without tzset) and glibc.
@clickhouse-gh

clickhouse-gh Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [68ff5ad]


AI Review

Summary

This PR fixes the musl-specific %Z portability problem by matching the Last-Modified zone as a literal GMT. That addresses the existing musl failure, but the parser is still hard-coded to one HTTP-date spelling, so the _time path remains incompatible with servers that emit the other legal date forms.

Findings

⚠️ Majors

  • [src/IO/ReadWriteBufferFromHTTP.cpp:805] Last-Modified is still parsed only as IMF-fixdate. Legal obs-date spellings such as Friday, 13-Mar-26 10:54:50 GMT and Fri Mar 13 10:54:50 2026 still leave res.last_modified unset, which means _time becomes NULL and the Last-Modified-based cache validation paths stay disabled for those responses. Suggested fix: accept the rfc850-date and asctime-date layouts too, or switch this code to a dedicated HTTP-date parser instead of a single strptime format.
Tests
  • ⚠️ Add a focused regression test that serves Last-Modified in at least one obs-date layout and verifies _time is populated. The existing 04104_url_time_virtual_column coverage only proves the IMF-fixdate form.
Final Verdict
  • Status: ⚠️ Request changes

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 23, 2026
@scanhex12 scanhex12 self-assigned this Jul 23, 2026
@thevar1able
thevar1able marked this pull request as ready for review July 23, 2026 14:32
String date_str = response.get("Last-Modified");
struct tm info{};
char * end = strptime(date_str.data(), "%a, %d %b %Y %H:%M:%S %Z", &info);
char * end = strptime(date_str.data(), "%a, %d %b %Y %H:%M:%S GMT", &info);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last-Modified is an HTTP-date, and recipients are supposed to accept the two obs-date layouts in addition to IMF-fixdate. Hard-coding a single "%a, %d %b %Y %H:%M:%S GMT" parse still leaves headers like Friday, 13-Mar-26 10:54:50 GMT and Fri Mar 13 10:54:50 2026 unparsed, so _time stays NULL and the Last-Modified-based cache validation paths remain disabled for servers that emit those legal spellings.

Can we fall back to the rfc850-date and asctime-date layouts here, or switch this to a dedicated HTTP-date parser, instead of supporting only IMF-fixdate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants