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

fix: resolve_bootstrap_node() not checking net_getipport() returned count correctly #2361

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Dec 2, 2022

fixes #2358

potential security issue, if the DNS server can cause this.


This change is Reviewable

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #2361 (88ffd1a) into master (9fae455) will decrease coverage by 68.15%.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##           master   #2361       +/-   ##
==========================================
- Coverage   78.10%   9.95%   -68.16%     
==========================================
  Files         141     141               
  Lines       31114   32503     +1389     
==========================================
- Hits        24302    3236    -21066     
- Misses       6812   29267    +22455     
Impacted Files Coverage Δ
toxcore/TCP_common.c 6.38% <0.00%> (-72.14%) ⬇️
toxcore/TCP_server.c 8.12% <0.00%> (-70.65%) ⬇️
toxcore/tox.c 7.80% <0.00%> (-56.32%) ⬇️
toxcore/crypto_core.c 33.88% <33.33%> (-59.83%) ⬇️
toxav/toxav_old.c 0.00% <0.00%> (-100.00%) ⬇️
auto_tests/file_saving_test.c 0.00% <0.00%> (-100.00%) ⬇️
auto_tests/conference_two_test.c 0.00% <0.00%> (-100.00%) ⬇️
auto_tests/invalid_tcp_proxy_test.c 0.00% <0.00%> (-100.00%) ⬇️
auto_tests/invalid_udp_proxy_test.c 0.00% <0.00%> (-100.00%) ⬇️
auto_tests/group_message_test.c 5.59% <0.00%> (-93.66%) ⬇️
... and 133 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sudden6
Copy link

sudden6 commented Dec 4, 2022

I checked the function and its documentation, which are quite confusing and probably led to this error. Can you improve the documentation so that it's more clear in which cases it returns 0?

@sudden6 sudden6 added this to the v0.2.19 milestone Dec 4, 2022
…ount correctly

doc: improve inline docu of return of net_getipport
@Green-Sky
Copy link
Member Author

@sudden6 i specified the return values in the inline docu
also rebased on master

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

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

@sudden6 sudden6 merged commit 88ffd1a into TokTok:master Jan 3, 2023
@Green-Sky Green-Sky deleted the fix_dns_count_0 branch March 8, 2023 12:38
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.

resolve_bootstrap_node assert hit
2 participants