Skip to content

test: Add a protocol dump test to generate initial fuzzer input.#2291

Merged
toktok-releaser merged 1 commit into
TokTok:masterfrom
iphydf:protodump
Apr 21, 2022
Merged

test: Add a protocol dump test to generate initial fuzzer input.#2291
toktok-releaser merged 1 commit into
TokTok:masterfrom
iphydf:protodump

Conversation

@iphydf

@iphydf iphydf commented Apr 15, 2022

Copy link
Copy Markdown
Member

This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Apr 15, 2022
@auto-add-label auto-add-label Bot added the test Adding missing tests, refactoring tests; no production code change label Apr 15, 2022
@iphydf iphydf force-pushed the protodump branch 3 times, most recently from f5df5f6 to 6d8a3d4 Compare April 15, 2022 21:39
@iphydf iphydf force-pushed the protodump branch 6 times, most recently from 25d69f7 to 784cc57 Compare April 16, 2022 01:27
@iphydf iphydf marked this pull request as ready for review April 16, 2022 01:28
@codecov

codecov Bot commented Apr 16, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2291 (50094b7) into master (3a5da35) will decrease coverage by 0.10%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #2291      +/-   ##
==========================================
- Coverage   79.02%   78.92%   -0.11%     
==========================================
  Files         127      127              
  Lines       24021    24063      +42     
==========================================
+ Hits        18983    18992       +9     
- Misses       5038     5071      +33     
Impacted Files Coverage Δ
toxcore/crypto_core.c 90.28% <ø> (ø)
auto_tests/auto_test_support.c 79.70% <33.33%> (-3.47%) ⬇️
toxcore/network.c 87.39% <97.43%> (+0.64%) ⬆️
toxcore/TCP_server.c 78.58% <0.00%> (-1.87%) ⬇️
toxcore/ping.c 84.56% <0.00%> (-1.24%) ⬇️
toxcore/Messenger.c 82.76% <0.00%> (-1.07%) ⬇️
toxcore/TCP_connection.c 69.45% <0.00%> (-0.87%) ⬇️
toxcore/friend_connection.c 83.25% <0.00%> (-0.70%) ⬇️
toxav/toxav.c 69.08% <0.00%> (-0.15%) ⬇️
toxcore/tox.c 64.65% <0.00%> (-0.08%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a5da35...50094b7. Read the comment docs.

@iphydf iphydf force-pushed the protodump branch 9 times, most recently from 2a9e389 to 13cf420 Compare April 17, 2022 23:28

@sudden6 sudden6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed 8 of 11 files at r1, 6 of 7 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)


testing/fuzzing/bootstrap_harness.cc, line 170 at r1 (raw file):

    uint8_t pub_key[TOX_PUBLIC_KEY_SIZE] = {0};

    const bool udp_success = tox_bootstrap(tox, "192.168.0.127", 33446, pub_key, nullptr);

Any reason to use IP addresses which might actually be used on the host machine? also how about reading them from fuzzer input? (just a thought and it will invalidate the corpus)


testing/fuzzing/e2e_fuzz_test.cc, line 131 at r1 (raw file):

}

static char tox_log_level_name(Tox_Log_Level level)

This function appears multiple times, make it constexpr and put it in a header?


testing/fuzzing/e2e_fuzz_test.cc, line 196 at r2 (raw file):

        // Move the clock forward a decent amount so all the time-based checks
        // trigger more quickly.
        sys.clock += 200;

I think this needs to be synced between all the fuzzers to be effective, right? This should be documented somewhere, and maybe a named constant. Also I know I was against this earlier, but I think now it really makes sense to read the time step from fuzzer input data so that all the behavior is contained in the corpus files.


testing/fuzzing/protodump.cc, line 188 at r2 (raw file):

{
    std::printf("%zu bytes: %s\n", recording.size(), filename);
    std::ofstream out_init(filename);

Might want to set the binary flag here

@iphydf iphydf left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @sudden6)


testing/fuzzing/bootstrap_harness.cc, line 170 at r1 (raw file):

Previously, sudden6 wrote…

Any reason to use IP addresses which might actually be used on the host machine? also how about reading them from fuzzer input? (just a thought and it will invalidate the corpus)

Reading them from the fuzzer input seems either risky or useless. In case something in toxcore structurally depends on the IP address, the fuzzer will have only a 1 in 4 billion chance of getting the right number. I don't think anything does depend on it, but even if it does, I don't think we'll find anything interesting from twiddling this particular number. If nothing indeed does depend on it, the input here only tests the "IP to string" and then "IP from string" functions, which are better tested outside the bootstrap fuzz test in a more specific test scenario.

Anyway, I've changed them back to (almost) the number you had here.


testing/fuzzing/e2e_fuzz_test.cc, line 131 at r1 (raw file):

Previously, sudden6 wrote…

This function appears multiple times, make it constexpr and put it in a header?

Done.


testing/fuzzing/e2e_fuzz_test.cc, line 196 at r2 (raw file):

Previously, sudden6 wrote…

I think this needs to be synced between all the fuzzers to be effective, right? This should be documented somewhere, and maybe a named constant. Also I know I was against this earlier, but I think now it really makes sense to read the time step from fuzzer input data so that all the behavior is contained in the corpus files.

Done.


testing/fuzzing/protodump.cc, line 188 at r2 (raw file):

Previously, sudden6 wrote…

Might want to set the binary flag here

Done.

@sudden6 sudden6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@toktok-releaser toktok-releaser merged commit 50094b7 into TokTok:master Apr 21, 2022
@iphydf iphydf deleted the protodump branch April 21, 2022 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Adding missing tests, refactoring tests; no production code change

Development

Successfully merging this pull request may close these issues.

3 participants