Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Tokenizer::int64() parsing of "0" when guessing base #1842

Closed
wants to merge 6 commits into from
Closed

Fix Tokenizer::int64() parsing of "0" when guessing base #1842

wants to merge 6 commits into from

Conversation

rousskov
Copy link
Contributor

@rousskov rousskov commented Jun 13, 2024 •
edited
Loading

Known bug victims in current code were tcp_outgoing_mark,
mark_client_packet, clientside_mark, and mark_client_connection
directives as well as client_connection_mark and (deprecated)
clientside_mark ACLs if they were configured to match a zero mark using
"0" or "0/..." syntax:

ERROR: configuration failure: NfMarkConfig: invalid value '0/10'...
exception location: NfMarkConfig.cc(23) getNfmark

Probably broken since 2014 commit 01f2137.

@rousskov
Fix Tokenizer::int64() parsing of "0" when guessing base
6a9d793 
Known bug victims in current code were client_connection_mark and
(deprecated) clientside_mark ACLs if they were configured to match a
zero mark using "0/..." syntax:

    ERROR: configuration failure: NfMarkConfig: invalid value '0/10'...
    exception location: NfMarkConfig.cc(23) getNfmark

Probably broken since 2014 commit 01f2137.
@rousskov rousskov mentioned this pull request Jun 13, 2024
Open
@rousskov rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jun 13, 2024
yadij
yadij requested changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

Just some missing tests.

src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
src/parser/Tokenizer.cc Show resolved Hide resolved
@yadij yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 14, 2024
rousskov
rousskov commented Jun 14, 2024
View reviewed changes
Copy link
Contributor Author

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

@yadij, I hope all of your concerns were addressed as of branch commit f9cc5eb. Please re-review.

src/parser/Tokenizer.cc Show resolved Hide resolved
src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
src/tests/testTokenizer.cc Outdated Show resolved Hide resolved
@rousskov rousskov requested a review from yadij June 14, 2024 16:01
@rousskov rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jun 14, 2024
@rousskov
Copy link
Contributor Author

rousskov commented Jun 14, 2024 •
edited
Loading

With @eduard-bagdasaryan's help, I added four directives to the two ACLs declared as known to be affected in this PR description: tcp_outgoing_mark, mark_client_packet, clientside_mark, and mark_client_connection. Their parser is parse_acl_nfmark() which calls Ip::NfMarkConfig::Parse() which ends up calling getNfmark() that calls int64() with zero base.

eduard-bagdasaryan
eduard-bagdasaryan reviewed Jun 14, 2024
View reviewed changes
src/tests/testTokenizer.cc
CPPUNIT_ASSERT_EQUAL(benchmark,rv);
CPPUNIT_ASSERT_EQUAL(SBuf(""), t.buf());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need also to test overflow with a zero-leading huge value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, better overflow tests would be useful, but they are outside the current PR scope IMO. Writing a good black-box leading-zero overflow test is not trivial. Adding a white-box leading-zero overflow test is not needed -- int64() loop is currently not counting the number of digits in any way and, hence, does not care if there are leading zeros AFAICT. Let's not add those tests (in this PR).

P.S. I added (useful) +0 and -0 tests because adding them was trivial. They are arguably in this PR scope.

eduard-bagdasaryan
eduard-bagdasaryan approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@yadij yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 21, 2024
@rousskov rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jun 21, 2024
@rousskov rousskov requested review from yadij and removed request for yadij June 21, 2024 16:26
@rousskov rousskov mentioned this pull request Jun 24, 2024
Closed
@rousskov
Copy link
Contributor Author

@yadij, please unblock this head-of-queue PR. It has no known blocking problems and has been waiting for your review for more than a week.

@yadij yadij removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jul 7, 2024
@yadij yadij added M-cleared-for-merge 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) and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Jul 7, 2024
@yadij
Copy link
Contributor

yadij commented Jul 7, 2024

FYI, resync with master to kick a retry of the failed tests on c409c33

squid-anubis pushed a commit that referenced this pull request Jul 7, 2024
@rousskov @squid-anubis
Fix Tokenizer::int64() parsing of "0" when guessing base (#1842)
8cc5e88 
Known bug victims in current code were tcp_outgoing_mark,
mark_client_packet, clientside_mark, and mark_client_connection
directives as well as client_connection_mark and (deprecated)
clientside_mark ACLs if they were configured to match a zero mark using
"0" or "0/..." syntax:

    ERROR: configuration failure: NfMarkConfig: invalid value '0/10'...
    exception location: NfMarkConfig.cc(23) getNfmark

Probably broken since 2014 commit 01f2137.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jul 7, 2024
@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 Jul 7, 2024
kinkie pushed a commit to kinkie/squid that referenced this pull request Oct 12, 2024
@rousskov @kinkie
Fix Tokenizer::int64() parsing of "0" when guessing base (squid-cache…
afc147f 
…#1842)

Known bug victims in current code were tcp_outgoing_mark,
mark_client_packet, clientside_mark, and mark_client_connection
directives as well as client_connection_mark and (deprecated)
clientside_mark ACLs if they were configured to match a zero mark using
"0" or "0/..." syntax:

    ERROR: configuration failure: NfMarkConfig: invalid value '0/10'...
    exception location: NfMarkConfig.cc(23) getNfmark

Probably broken since 2014 commit 01f2137.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eduard-bagdasaryan eduard-bagdasaryan eduard-bagdasaryan approved these changes

@yadij yadij yadij approved these changes

Assignees
No one assigned
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rousskov @yadij @eduard-bagdasaryan @squid-anubis