search

TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.21k stars 280 forks source link

fix: Correctly pass extended public keys to group moderation code. #2689

Closed iphydf closed 4 months ago

iphydf commented 4 months ago

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

This change is Reviewable

codecov[bot] commented 4 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nurupo commented 4 months ago

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.

JFreegman commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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