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: Correctly pass extended public keys to group moderation code. #2689

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 15, 2024

It happens to work, but isn't clean, and static analysers found this (SonarCloud and Coverity both detected this).


This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Feb 15, 2024
@iphydf iphydf marked this pull request as ready for review February 15, 2024 10:52
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

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

Comparison is base (021db70) 73.04% compared to head (710eb67) 73.10%.

Files Patch % Lines
toxcore/group_chats.c 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2689      +/-   ##
==========================================
+ Coverage   73.04%   73.10%   +0.06%     
==========================================
  Files         149      149              
  Lines       30516    30517       +1     
==========================================
+ Hits        22289    22309      +20     
+ Misses       8227     8208      -19     

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

It happens to work, but isn't clean, and static analysers found this
(SonarCloud and Coverity both detected this).
@nurupo
Copy link
Member

nurupo commented Feb 15, 2024

SonarCloud has such high noise ratio due to complaining about auto_tests, I'm surprised you found anything useful on there. Is there some trick to removing all the noise?

Also, where can we find the URL to see Coverity results? Doesn't seem like those link to anything https://github.com/TokTok/c-toxcore/actions/workflows/coverity-scan.yml, or if they do it's not very obvious.

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.

Ok, found it on Sonar, and yeah, this is a bit nasty but it does happen to work.

@iphydf iphydf merged commit 710eb67 into TokTok:master Feb 15, 2024
60 of 61 checks passed
@iphydf iphydf deleted the array-oob branch February 15, 2024 13:25
@JFreegman
Copy link
Member

What was corrected? It looks like a cleanup/refactor, which I'm fine with, as the whole putting two keys in one buffer idea was always a bad one, but the PR title and branch name are both misleading. This code was correct and has been working fine for years, despite being ugly. It didn't "happen" to work. It worked because it's been reviewed and tested ad nauseum.

@iphydf
Copy link
Member Author

iphydf commented Feb 15, 2024

The code was broken by me in #2672. I corrected it in this PR. The PR title and branch name are both appropriate and not misleading. It indeed "happened" to work, because the sig member happened to follow the enc member in memory. This is not only ugly, but also undefined behaviour that just "happened" to work.

@JFreegman
Copy link
Member

Ah my mistake, I didn't realize that other PR had already been merged, and thought the "fix" was just removing usage of a double buffer (which is prone to mistakes). Although, it would have been better to mention exactly what was broken and when in the original PR message, as my initial thought was that we had a long-standing buffer overrun.

@nurupo
Copy link
Member

nurupo commented Feb 15, 2024

@JFreegman the Sonar link I posted above is helpful in pointing out what the problem was. We basically were writing two public keys worth of data (enc+sig) into a single public key (enc) array, overflowing it. Luckily we were overflowing into another array within the same struct -- the sig public key array, but it's still likely an ub.

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