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: Don't use memcmp to compare IP_Ports. #2605

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 26, 2024

memcmp compares padding bytes as well, which may be arbitrary or uninitialised.


This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Jan 26, 2024
@iphydf iphydf force-pushed the ipport-cmp branch 2 times, most recently from 663172a to 5c7854e Compare January 26, 2024 16:16
@iphydf iphydf marked this pull request as ready for review January 26, 2024 16:16
@iphydf iphydf force-pushed the ipport-cmp branch 3 times, most recently from 9ceb3fa to 5f6cd74 Compare January 26, 2024 16:19
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (d94246a) 73.72% compared to head (39aadf8) 73.60%.

Files Patch % Lines
toxcore/network.c 61.76% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2605      +/-   ##
==========================================
- Coverage   73.72%   73.60%   -0.13%     
==========================================
  Files         148      148              
  Lines       30369    30407      +38     
==========================================
- Hits        22390    22380      -10     
- Misses       7979     8027      +48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

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

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


toxcore/network.h line 365 at r1 (raw file):

 */
non_null()
int ipport_cmp_handler(const void *a, const void *b, size_t size);

What do the return values indicate?


toxcore/network.c line 1460 at r1 (raw file):

    }
    // If family isn't ipv4, assume ipv6 and compare as many bytes as we can.
    return ip6_cmp(&a->ip.v6, &b->ip.v6);

Why do we assume it's ip6 here when there are more than two possibilities?

Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

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


toxcore/network.c line 1441 at r2 (raw file):

static int ip4_cmp(const IP4 *a, const IP4 *b)
{
    return a->uint32 - b->uint32;

Can't this value overflow the return type of int? Even if int is 32-bit, since it's signed, it has half the range of uint32. Maybe return -1, 0, 1 instead?


toxcore/network.c line 1448 at r2 (raw file):

{
    if (a->uint64[0] != b->uint64[0]) {
        return a->uint64[0] - b->uint64[0];

Can't this value overflow the return type of int? Maybe return -1, 0, 1 instead?


toxcore/network.c line 1450 at r2 (raw file):

        return a->uint64[0] - b->uint64[0];
    }
    return a->uint64[1] - b->uint64[1];

Can't this value overflow the return type of int? Maybe return -1, 0, 1 instead?

@JFreegman
Copy link
Member

toxcore/network.c line 1441 at r2 (raw file):

Previously, nurupo wrote…

Can't this value overflow the return type of int? Even is int is 32-bit, since it's signed, it has half the range of uint32. Maybe return -1, 0, 1 instead?

Does an overflow even matter? We only care that the result isn't 0 (I believe?)

Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @JFreegman and @nurupo)


toxcore/network.h line 365 at r1 (raw file):

Previously, JFreegman wrote…

What do the return values indicate?

Added docs for it.


toxcore/network.c line 1460 at r1 (raw file):

Previously, JFreegman wrote…

Why do we assume it's ip6 here when there are more than two possibilities?

Meh :). You're right, this is more proper.


toxcore/network.c line 1441 at r2 (raw file):

Previously, JFreegman wrote…

Does an overflow even matter? We only care that the result isn't 0 (I believe?)

Doesn't really matter, but fixed anyway.


toxcore/network.c line 1448 at r2 (raw file):

Previously, nurupo wrote…

Can't this value overflow the return type of int? Maybe return -1, 0, 1 instead?

Done.


toxcore/network.c line 1450 at r2 (raw file):

Previously, nurupo wrote…

Can't this value overflow the return type of int? Maybe return -1, 0, 1 instead?

Done.

Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @JFreegman)

@iphydf iphydf force-pushed the ipport-cmp branch 3 times, most recently from 902c9de to 3171c82 Compare January 26, 2024 18:51
Copy link
Member

@JFreegman JFreegman 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 7 files at r3, 2 of 2 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained

@iphydf iphydf force-pushed the ipport-cmp branch 3 times, most recently from 5a2d763 to 317ac88 Compare January 29, 2024 13:57
`memcmp` compares padding bytes as well, which may be arbitrary or
uninitialised.
@iphydf iphydf merged commit 39aadf8 into TokTok:master Jan 29, 2024
57 of 58 checks passed
@iphydf iphydf deleted the ipport-cmp branch January 29, 2024 14:43
This pull request was closed.
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