Skip to content

Bug 5552: Buffer overflow when parsing deny_info NNN w/o URL - #2481

Closed
rousskov wants to merge 1 commit into
squid-cache:masterfrom
measurement-factory:SQUID-111-bug5552-deny-info-parsing
Closed

Bug 5552: Buffer overflow when parsing deny_info NNN w/o URL#2481
rousskov wants to merge 1 commit into
squid-cache:masterfrom
measurement-factory:SQUID-111-bug5552-deny-info-parsing

Conversation

@rousskov

Copy link
Copy Markdown
Contributor
deny_info 307

The bug affected deny_info status codes 200-599. The bug was introduced
with the initial support for custom status codes (2011 commit aed9a15).

    deny_info 307

The bug affected deny_info status codes 200-599. The bug was introduced
with the initial support for custom status codes (2011 commit aed9a15).
Comment thread src/errorpage.cc
filenameOrUri = page_name;
}

const auto looksLikeUrl = [](const char * const name) { return name && strchr(name, ':'); };

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.

Since all current callers supply the same filenameOrUri argument, and that argument does not change after this line, we can make looksLikeUrl a boolean constant, but a lambda makes this (quite messy and difficult-to-follow!) code more readable IMO.

This lambda will be called twice, but an optimizing compiler may be able to avoid the second call, and deny_info configuration parsing is not a particularly performance-sensitive functionality.

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.

Wouldn't this belong better in anyp/Uri?

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.

Wouldn't this belong better in anyp/Uri?

This specific heuristic may not be suitable for safe reuse in some other contexts looking for URIs. This simple lambda is context-specific.

There may be other contexts were the underlying heuristic would work correctly, but we would need to find a more suitable name (to fend off callers that should use another logic). Something like ContainsColon() is too low-level and not worth wrapping/naming!

Before we settle on the new name and other details, we would also need to find other suitable callers. Then we will need to convert them.

I am not against making this one-liner more reusable, but would rather not spend my time on that right now, especially in a surgical bug-fixing PR.

@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.

@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 added S-waiting-for-author author action is expected (and usually required) backport-to-v7 maintainer has approved these changes for v7 backporting and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Aug 26, 2026
@rousskov rousskov added S-could-use-an-approval An approval may speed this PR merger (but is not required) and removed S-waiting-for-author author action is expected (and usually required) labels Aug 26, 2026
@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Sep 4, 2026
squid-anubis pushed a commit that referenced this pull request Sep 4, 2026
    deny_info 307

The bug affected deny_info status codes 200-599. The bug was introduced
with the initial support for custom status codes (2011 commit aed9a15).
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Sep 4, 2026
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 5, 2026
@squidadm squidadm removed the backport-to-v7 maintainer has approved these changes for v7 backporting label Sep 5, 2026
@squidadm

squidadm commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

queued for backport to v7

yadij pushed a commit that referenced this pull request Sep 6, 2026
    deny_info 307

The bug affected deny_info status codes 200-599. The bug was introduced
with the initial support for custom status codes (2011 commit aed9a15).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-merged https://github.com/measurement-factory/anubis#pull-request-labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants