Skip to content

Bug 5553: Segfault when parsing valueless request_header_add - #2482

Open
rousskov wants to merge 1 commit into
squid-cache:masterfrom
measurement-factory:SQUID-111-bug5552-header-add-parsing
Open

Bug 5553: Segfault when parsing valueless request_header_add#2482
rousskov wants to merge 1 commit into
squid-cache:masterfrom
measurement-factory:SQUID-111-bug5552-header-add-parsing

Conversation

@rousskov

Copy link
Copy Markdown
Contributor
request_header_add X-Test-Field-Name

Some STL implementations segfault when the std::string(s) constructor
is given a nil pointer. The exact behavior is undefined by the standard.
The constructor API requires a null-terminated (i.e. non-nil) string.

Also delayed nlf object creation (until it becomes needed) to avoid
wasting resources and code lines on an unused object. This code still
leaks those objects on nlf->parse() failures (i.e. C++ exceptions),
but a proper fix for that old bug deserves a dedicated change.

The same bug affects the reply_header_add configuration directive.

    request_header_add X-Test-Field-Name

Some STL implementations segfault when the `std::string(s)` constructor
is given a nil pointer. The exact behavior is undefined by the standard.
The constructor API requires a null-terminated (i.e. non-nil) string.

Also delayed `nlf` object creation (until it becomes needed) to avoid
wasting resources and code lines on an unused object. This code still
leaks those objects on `nlf->parse()` failures (i.e. C++ exceptions),
but a proper fix for that old bug deserves a dedicated change.

The same bug affects the `reply_header_add` configuration directive.

@rousskov rousskov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I asked @k-furman, the bug reporter, to test this fix.

Comment thread src/cache_cf.cc
hwa.valueFormat = nlf;
} else
delete nlf;
// XXX: We leak memory when parse() throws due to a parsing failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This memory leak is an old out-of-scope bug. This PR does not make it worse.

@rousskov rousskov added the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Aug 24, 2026

@kinkie kinkie left a comment

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.

LGTM

@kinkie kinkie added the backport-to-v7 maintainer has approved these changes for v7 backporting label Aug 26, 2026
squid-anubis pushed a commit that referenced this pull request Aug 26, 2026
    request_header_add X-Test-Field-Name

Some STL implementations segfault when the `std::string(s)` constructor
is given a nil pointer. The exact behavior is undefined by the standard.
The constructor API requires a null-terminated (i.e. non-nil) string.

Also delayed `nlf` object creation (until it becomes needed) to avoid
wasting resources and code lines on an unused object. This code still
leaks those objects on `nlf->parse()` failures (i.e. C++ exceptions),
but a proper fix for that old bug deserves a dedicated change.

The same bug affects the `reply_header_add` configuration directive.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-v7 maintainer has approved these changes for v7 backporting M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants