Skip to content

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

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

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

2 participants