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.

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

swim: fix debug assertion abort in proto decode #6662

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

SweetVishnya
Copy link
Contributor

assert(cur < end) after mp_load_u8 in mp_check_binl was failing

This PR fixes abort found with libFuzzer target in #6627

@coveralls
Copy link

coveralls commented Nov 30, 2021

Coverage Status

Coverage increased (+0.1%) to 84.778% when pulling c0cba4c on SweetVishnya:fix-swim-abort into 04b2230 on tarantool:master.

Copy link
Collaborator

@Gerold103 Gerold103 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Please, add a test to unit/swim_proto.c. All patches should have a test when possible. Here it should be. Libfuzzer is rather a helper tool than a regular test running on people's machines. Also I can't run it anyway, so I can't formally validate the fix.

@SweetVishnya
Copy link
Contributor Author

I managed to add a regression test.

Output without patch applied:

./swim_proto.test

	*** main ***
1..3
	*** swim_test_member_def ***
    1..13
    ok 1 - not map header
    ok 2 - not uint member key
    ok 3 - too big member key
    ok 4 - STATUS is not uint
    ok 5 - invalid STATUS
    ok 6 - invalid address
    ok 7 - bad port
    ok 8 - unexpected buffer end
    ok 9 - unexpected buffer end
    ok 10 - uuid is nil/undefined
    ok 11 - port is 0/undefined
    ok 12 - normal member def
swim_proto.test: /home/vishnya/fwork/tarantool/src/lib/msgpuck/msgpuck.h:2052: ptrdiff_t mp_check_binl(const char *, const char *): Assertion `cur < end' failed.
[1]    17720 abort (core dumped)  ./swim_proto.test

Output after patch is applied:

./swim_proto.test

	*** main ***
1..3
	*** swim_test_member_def ***
    1..13
    ok 1 - not map header
    ok 2 - not uint member key
    ok 3 - too big member key
    ok 4 - STATUS is not uint
    ok 5 - invalid STATUS
    ok 6 - invalid address
    ok 7 - bad port
    ok 8 - unexpected buffer end
    ok 9 - unexpected buffer end
    ok 10 - uuid is nil/undefined
    ok 11 - port is 0/undefined
    ok 12 - normal member def
    ok 13 - zero length bin
ok 1 - subtests
	*** swim_test_member_def: done ***
	*** swim_test_meta ***
    1..8
    ok 1 - not map header
    ok 2 - not uint meta key
    ok 3 - unknown meta key
    ok 4 - unexpected end
    ok 5 - invalid version
    ok 6 - port is 0/undefined
    ok 7 - version is 0/undefined
    ok 8 - normal meta
ok 2 - subtests
	*** swim_test_meta: done ***
	*** swim_test_route ***
    1..5
    ok 1 - route was expected, but map is too short
    ok 2 - no route map
    ok 3 - empty route map
    ok 4 - zero addresses
    ok 5 - normal route
ok 3 - subtests
	*** swim_test_route: done ***
	*** main: done ***

@Gerold103 Gerold103 self-requested a review December 6, 2021 21:32
Copy link
Collaborator

@Gerold103 Gerold103 left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! Please, merge these 3 commits into one. We usually do fix + test in one commit.

assert(cur < end) after mp_load_u8 in mp_check_binl was failing
@SweetVishnya
Copy link
Contributor Author

I squashed commits and rebased on master.

@Gerold103 Gerold103 self-requested a review December 8, 2021 22:52
@Gerold103 Gerold103 merged commit d1a60af into tarantool:master Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants