Skip to content

Commit

Permalink
refactor: Make prune_gc_sanctions_list more obviously correct.
Browse files Browse the repository at this point in the history
`target_ext_pk` is uninitialised before the loop, and is definitely
initialised after the loop *iff* `sanction` isn't NULL. If it is NULL,
then `target_ext_pk` is not read. This is verifiable, but not obvious.
PVS Studio isn't smart enough to figure that out, so it complains. I
consider it a valid complaint, and the new code is clearer: we now call
the processing part of the function directly with initialised values
instead of relying on that initialisation coinciding with `sanction`s
nullity.
  • Loading branch information
iphydf committed Feb 9, 2024
1 parent 3ba7a0d commit f058103
Showing 1 changed file with 30 additions and 30 deletions.
60 changes: 30 additions & 30 deletions toxcore/group_chats.c
Original file line number Diff line number Diff line change
Expand Up @@ -1086,43 +1086,16 @@ static bool prune_gc_mod_list(GC_Chat *chat)
&& update_gc_topic(chat, public_sig_key);
}

/** @brief Removes the first found offline sanctioned peer from the sanctions list and sends the
* event to the rest of the group.
*
* @retval false on failure or if no presently sanctioned peer is offline.
*/
non_null()
static bool prune_gc_sanctions_list(GC_Chat *chat)
static bool prune_gc_sanctions_list_inner(
GC_Chat *chat, const Mod_Sanction *sanction,
const uint8_t target_ext_pk[ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE])
{
if (chat->moderation.num_sanctions == 0) {
return true;
}

const Mod_Sanction *sanction = nullptr;
uint8_t target_ext_pk[ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE];

for (uint16_t i = 0; i < chat->moderation.num_sanctions; ++i) {
const int peer_number = get_peer_number_of_enc_pk(chat, chat->moderation.sanctions[i].target_public_enc_key, true);

if (peer_number == -1) {
sanction = &chat->moderation.sanctions[i];
memcpy(target_ext_pk, sanction->target_public_enc_key, ENC_PUBLIC_KEY_SIZE);
memcpy(target_ext_pk + ENC_PUBLIC_KEY_SIZE, sanction->setter_public_sig_key, SIG_PUBLIC_KEY_SIZE);
break;
}
}

if (sanction == nullptr) {
return false;
}

if (!sanctions_list_remove_observer(&chat->moderation, sanction->target_public_enc_key, nullptr)) {
LOGGER_WARNING(chat->log, "Failed to remove entry from observer list");
return false;
}

sanction = nullptr;

uint8_t data[MOD_SANCTIONS_CREDS_SIZE];
const uint16_t length = sanctions_creds_pack(&chat->moderation.sanctions_creds, data);

Expand All @@ -1139,6 +1112,33 @@ static bool prune_gc_sanctions_list(GC_Chat *chat)
return true;
}

/** @brief Removes the first found offline sanctioned peer from the sanctions list and sends the
* event to the rest of the group.
*
* @retval false on failure or if no presently sanctioned peer is offline.
*/
non_null()
static bool prune_gc_sanctions_list(GC_Chat *chat)
{
if (chat->moderation.num_sanctions == 0) {
return true;

Check warning on line 1124 in toxcore/group_chats.c

View check run for this annotation

Codecov / codecov/patch

toxcore/group_chats.c#L1123-L1124

Added lines #L1123 - L1124 were not covered by tests
}

for (uint16_t i = 0; i < chat->moderation.num_sanctions; ++i) {
const int peer_number = get_peer_number_of_enc_pk(chat, chat->moderation.sanctions[i].target_public_enc_key, true);

Check warning on line 1128 in toxcore/group_chats.c

View check run for this annotation

Codecov / codecov/patch

toxcore/group_chats.c#L1127-L1128

Added lines #L1127 - L1128 were not covered by tests

if (peer_number == -1) {
const Mod_Sanction *sanction = &chat->moderation.sanctions[i];
uint8_t target_ext_pk[ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE];
memcpy(target_ext_pk, sanction->target_public_enc_key, ENC_PUBLIC_KEY_SIZE);
memcpy(target_ext_pk + ENC_PUBLIC_KEY_SIZE, sanction->setter_public_sig_key, SIG_PUBLIC_KEY_SIZE);
return prune_gc_sanctions_list_inner(chat, sanction, target_ext_pk);

Check warning on line 1135 in toxcore/group_chats.c

View check run for this annotation

Codecov / codecov/patch

toxcore/group_chats.c#L1130-L1135

Added lines #L1130 - L1135 were not covered by tests
}
}

return false;

Check warning on line 1139 in toxcore/group_chats.c

View check run for this annotation

Codecov / codecov/patch

toxcore/group_chats.c#L1139

Added line #L1139 was not covered by tests
}

/** @brief Size of peer data that we pack for transfer (nick length must be accounted for separately).
* packed data consists of: nick length, nick, and status.
*/
Expand Down

0 comments on commit f058103

Please sign in to comment.