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

Refactor: Extract shared key cache into separate file #2317

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

sudden6
Copy link

@sudden6 sudden6 commented May 22, 2022

This removes the old and badly documented/designed shared key cache from DHT.c and moves the implemenation to a separate file. This makes it more testable and allows a better overview of how many caches exist (currently 7, total ~0,5MB). Additionally I think this fixes several missed crypto_memzero calls.


This change is Reviewable

@sudden6 sudden6 changed the title Refactor/shared key cache Refactor: Extract shared key cache into separate file May 23, 2022
@auto-add-label auto-add-label bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label May 23, 2022
@sudden6 sudden6 added this to the v0.2.19 milestone May 23, 2022
@sudden6 sudden6 force-pushed the refactor/shared_key_cache branch from 5454a89 to 5ee8e1f Compare May 27, 2022 23:56
@codecov
Copy link

codecov bot commented May 28, 2022

Codecov Report

Merging #2317 (de97532) into master (e0b00d3) will increase coverage by 0.09%.
The diff coverage is 91.73%.

@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
+ Coverage   78.08%   78.17%   +0.09%     
==========================================
  Files         140      141       +1     
  Lines       31053    31097      +44     
==========================================
+ Hits        24247    24311      +64     
+ Misses       6806     6786      -20     
Impacted Files Coverage Δ
toxcore/announce.c 58.80% <66.66%> (+0.41%) ⬆️
toxcore/onion.c 85.71% <87.50%> (+0.60%) ⬆️
toxcore/onion_announce.c 85.76% <87.50%> (+0.31%) ⬆️
toxcore/shared_key_cache.c 94.64% <94.64%> (ø)
toxcore/DHT.c 84.74% <100.00%> (-0.34%) ⬇️
toxcore/net_crypto.c 87.37% <100.00%> (+0.01%) ⬆️
toxcore/ping.c 86.92% <100.00%> (+0.50%) ⬆️
auto_tests/group_invite_test.c 98.44% <0.00%> (-0.78%) ⬇️
toxav/toxav.c 69.08% <0.00%> (-0.15%) ⬇️
... and 11 more

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

@sudden6 sudden6 changed the title Refactor: Extract shared key cache into separate file [WIP] Refactor: Extract shared key cache into separate file May 28, 2022
@sudden6 sudden6 force-pushed the refactor/shared_key_cache branch from baa56e3 to d6cc0c5 Compare May 29, 2022 09:36
@sudden6 sudden6 changed the title [WIP] Refactor: Extract shared key cache into separate file Refactor: Extract shared key cache into separate file Aug 7, 2022
@sudden6 sudden6 force-pushed the refactor/shared_key_cache branch 6 times, most recently from 2a8f10f to 5b39a26 Compare October 2, 2022 11:03
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 13 of 14 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @sudden6)


toxcore/announce.c line 438 at r1 (raw file):

    VLA(uint8_t, plain, plain_len);

    const uint8_t* shared_key = shared_key_cache_lookup(announce->shared_keys, data_public_key);

shared_key_cache_lookup() can return null. Wouldn't it be better to do a null check on the return value immediately and log any errors, instead of propagating the null pointer to decrypt_data_symmetric() and obfuscating the error?


toxcore/shared_key_cache.h line 20 at r1 (raw file):

/**
 * @brief Initializes a new shared key cache

Missing period (a couple more in this file)

Copy link
Author

@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.

Reviewable status: 0 of 1 approvals obtained (waiting on @JFreegman)


toxcore/announce.c line 438 at r1 (raw file):

Previously, JFreegman wrote…

shared_key_cache_lookup() can return null. Wouldn't it be better to do a null check on the return value immediately and log any errors, instead of propagating the null pointer to decrypt_data_symmetric() and obfuscating the error?

Done.


toxcore/shared_key_cache.h line 20 at r1 (raw file):

Previously, JFreegman wrote…

Missing period (a couple more in this file)

Done.

@sudden6 sudden6 assigned JFreegman and unassigned sudden6 Nov 21, 2022
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 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@sudden6 sudden6 force-pushed the refactor/shared_key_cache branch 3 times, most recently from ca4fd96 to 479875b Compare November 22, 2022 22:41
The exisiting implementation is not clearly documented and used by
multiple modules.
@sudden6 sudden6 merged commit de97532 into TokTok:master Nov 23, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants