Fix truncated HTTP version in log field unmarshalling#13236
Open
masaori335 wants to merge 2 commits into
Open
Conversation
unmarshal_http_version did not advance the buffer pointer past the
minor-version digits before computing the output length, so logged
HTTP versions dropped the minor number ("HTTP/1." instead of
"HTTP/1.1"). This is a regression from apache#11437, which changed val_len
to a pointer subtraction without advancing past the second integer.
These functions have no callers, and several are declared without ever being defined; none are paired with a log field: - unmarshal_http_text: no marshal counterpart; referenced only by its own declaration and (former) definition. - marshal_milestone_fmt_squid/_netscape/_date/_time: declared but never defined. Only _fmt_sec and _fmt_ms are implemented and used; the date/time/netscape/squid formatting happens at unmarshal time. - unmarshal_client_protocol_stack: declared but never defined or wired into any log field.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a log unmarshalling bug in Apache Traffic Server’s logging subsystem where LogAccess::unmarshal_http_version() could truncate the HTTP minor version (e.g. producing HTTP/1. instead of HTTP/1.1), impacting the sshv/csshv log fields. It also removes unused marshal/unmarshal APIs to keep LogAccess leaner.
Changes:
- Fix
LogAccess::unmarshal_http_version()to correctly advance the write pointer after unmarshalling the minor version. - Add a unit test that validates the unmarshalled HTTP version preserves both major and minor components.
- Remove unused
LogAccessmarshal/unmarshal declarations (and remove the unusedunmarshal_http_text()implementation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/proxy/logging/unit-tests/test_LogAccess.cc |
Adds a regression test ensuring unmarshal_http_version() renders HTTP/<major>.<minor> without truncation. |
src/proxy/logging/LogAccess.cc |
Fixes the truncation bug by advancing p after writing the minor version; removes unused unmarshal_http_text() implementation. |
include/proxy/logging/LogAccess.h |
Removes unused marshal/unmarshal declarations from the LogAccess interface. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I found
LogAccess::unmarshal_http_versionhas a bug, it's truncated - e.g. "HTTP/1." instead of "HTTP/1.1".This affects only
sshvandcsshv. (Clarification:cqpvandsqpvare not affected)Also, remove several unused marshal/unmarshal functions.