Make conferences persist across restarts

[robinlinden]

New PR with @isotoxin's code from #263. The scope of this PR is cleanup.

---

This change is 

[sudden6]

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 32 at r1 (raw file):

#include "util.h"

typedef ptrdiff_t aint;

would be better to use C99 standard types here as in the rest of the code

---

toxcore/group.c, line 421 at r1 (raw file):

                if (empty_slots_in_list > 0) {

                    aint empty_slot = -1;

same

---

Comments from Reviewable

[robinlinden]

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 32 at r1 (raw file):

Previously, sudden6 wrote…

would be better to use C99 standard types here as in the rest of the code

Working on it. There are a ton of aints.

---

toxcore/group.c, line 421 at r1 (raw file):

Previously, sudden6 wrote…

same

Done.

---

Comments from Reviewable

[iphydf]

Semantics changed: previously it was < 0.

[robinlinden]

Done.

[iphydf]

@robinlinden can you squash all your commits, so I can see whether your changes are correct? Also, is this PR ready for review? You don't need to make it perfect before review. The idea was that we would take the code as-is, and collaboratively improve it through several rounds of review. Your task is just to evaluate and make the changes requested by the reviewers (one of which may be also you, but we should discuss the changes even if you make them).

[robinlinden]

I made a logic change here. Previously the function returned success if it couldn't allocate space for a new nickname.

[robinlinden]

It is ready for review. Most of what I've done has been removing aints and replacing them with other suitable types. While there are still a lot of them left, this is ready for review. I'll stop making changes without posting comments about it first so we can discuss them.

[iphydf]

Review status: 0 of 10 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 34 at r2 (raw file):

typedef ptrdiff_t aint;

#define CLOSEST( g, i ) (((g)->closest_peers_entry & (1ull << (i))) != 0)

Remove extra spacing after ( and before ).

---

toxcore/group.c, line 41 at r2 (raw file):

    UNS_TEMP,
    UNS_FOREVER,

Remove empty line.

---

toxcore/group.c, line 54 at r2 (raw file):

    GROUP_MESSAGE_KILL_PEER_ID = 17,
    GROUP_MESSAGE_NAME_ID = 48,
    GROUP_MESSAGE_TITLE_ID = 49

• Add , and end of last enumerator.
• Align all = in enumerations.

---

toxcore/group.c, line 261 at r3 (raw file):

{
    uint16_t peer_number;
    randombytes((uint8_t *)&peer_number, 2);

Create a new function in crypto_core named random_u16 and call that.

---

toxcore/group.c, line 295 at r3 (raw file):

    }

    return (cmp1 - cmp2);

Remove superfluous () around return value expression.

---

toxcore/group.c, line 300 at r3 (raw file):

static bool closest(const Group_c *g, const uint8_t i)
{
    return ((g->closest_peers_entry & (1ull << i)) != 0);

Remove superfluous () around return value expression.

---

toxcore/group.c, line 306 at r3 (raw file):

{
    g->closest_peers_entry |= (1ull << i);
    return;

Remove no-op return.

---

toxcore/group.c, line 324 at r3 (raw file):

    }

    size_t index = DESIRED_CLOSE_CONNECTIONS;

Above, the type used for something counting towards DESIRED_CLOSE_CONNECTION is uint8_t, but now it's size_t. Which one should it be? Does index exceed the range of uint8_t?

---

toxcore/group.c, line 355 at r3 (raw file):

    if (index < DESIRED_CLOSE_CONNECTIONS) {
        aint rmpeer = g->closest_peers[index];
        g->closest_peers[index] = (uint16_t)peerindex;

peerindex is already a uint16_t. Remove identity cast.

---

toxcore/group.c, line 392 at r3 (raw file):

}

static void apply_changes_in_peers(Group_Chats *g_c, int32_t groupnumber, void *userdata)

This function is 257 lines long. That is too long. Please break it into logical units.

---

toxcore/group.c, line 651 at r3 (raw file):

}

static void send_peer_nums(const Group_Chats *g_c, int32_t groupnumber, aint friendcon_id, uint16_t other_group_num);

Move this declaration to the top together with all the other static function declarations.

---

toxcore/group.c, line 653 at r3 (raw file):

static void send_peer_nums(const Group_Chats *g_c, int32_t groupnumber, aint friendcon_id, uint16_t other_group_num);

static void connect_to_closest(Group_Chats *g_c, int32_t groupnumber, void *userdata)

Perhaps break this function into two or three.

---

toxcore/group.c, line 665 at r3 (raw file):

    }

    size_t i;

numpeers is uint32_t. Why is this loop variable size_t? It is used twice. Once as numpeers loop variable, and once as DESIRED_CLOSE_CONNECTIONS, which was previously looped over with uint8_t.

---

toxcore/group.c, line 688 at r3 (raw file):

                }

                size_t k;

Another size_t that should(?) be uint8_t.

---

toxcore/group.c, line 782 at r3 (raw file):

    }

    size_t i;

numjoinpeers is uint32_t. Is there a reason for this to be size_t?

---

toxcore/group.c, line 815 at r3 (raw file):

static void need_send_peers(Group_c *g)
{
    size_t i;

uint32_t?

---

toxcore/group.c, line 828 at r3 (raw file):

    }

    aint peer_index = peer_in_chat(g, real_pk);

This is definitely wrong. aint can be a 32 bit signed type. The function peer_in_chat returns int64_t, because it returns either a 32 bit unsigned number or -1. This means that peer_in_chat is written with the intention of using the entire 32 bit range for its return value. This in turn means that you can get a signed integer overflow.

---

Comments from Reviewable

[iphydf]

Reviewed 8 of 10 files at r1, 2 of 2 files at r2.
Review status: 9 of 10 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[robinlinden]

Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 34 at r2 (raw file):

Previously, iphydf wrote…

Remove extra spacing after ( and before ).

I switched these macros over to being functions.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 191 at r3 (raw file):

    for (i = 0; i < chat->numpeers; ++i) {
        if (chat->peers[i].friendcon_id != -2 && id_equal(chat->peers[i].real_pk, real_pk)) {

What's -2?

---

toxcore/group.c, line 935 at r3 (raw file):

    if (g->peer_on_leave) {
        g->peer_on_leave(g->object, (int)groupnumber, peer->object);

Instead of casting here, make peer_on_leave take an int32_t.

---

toxcore/group.c, line 2388 at r3 (raw file):

        if (send_packet_group_peer(g_c->fr_c, friendcon_id, PACKET_ID_DIRECT_CONFERENCE, other_group_num, packet,
                                   (p - packet))) {
            sent = i;

sent is set but never read. @isotoxin what was the intention here?

---

toxcore/group.c, line 2394 at r3 (raw file):

    if (g->title_len) {

        uint8_t Packet[1 + MAX_NAME_LENGTH];

Local variables should be lowercase.

---

toxcore/group.c, line 3973 at r3 (raw file):

}

#define put16(v) { *((uint16_t *)data) = host_tolendian16( (uint16_t)(v) ); data += sizeof( uint16_t ); }

While this looks cute, it's slightly confusing to have "functions" mutating variables in the lexical environment. Prefer to make these two functions and calling them like data = put16(data, v);.

---

toxcore/group.c, line 3981 at r3 (raw file):

    uint16_t *num = (uint16_t *)data;
    *num = 0;

Are we sure this cast and dereference is valid? Is data 2-byte aligned?

---

toxcore/group.h, line 55 at r3 (raw file):

    uint8_t     *nick;

    uint32_t    last_message_number[9]; /* 9 - number of group messages */

Why was this 9 again? I keep forgetting, which means we need a named constant with an explanation as to why it's 9.

---

toxcore/group.h, line 104 at r3 (raw file):

    void (*group_on_delete)(void *, int);

    uint8_t identifier[GROUP_IDENTIFIER_LENGTH];

Add a comment about what's in here: the type (what is type?) and uid (32 random bytes).

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 979 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

I made a logic change here. Previously the function returned success if it couldn't allocate space for a new nickname.

@isotoxin can you verify that this semantic change is correct?

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 919 at r3 (raw file):

    }

    Group_Peer *peer = g->peers + peer_index;

&g->peers[peer_index]

---

toxcore/group.c, line 1375 at r3 (raw file):

 * return -3 if we are not connected to the group chat.
 */
int group_peernumber_is_ours(const Group_Chats *g_c, int groupnumber, int peer_index)

@isotoxin what's the difference between peer_index and peernumber?

---

Comments from Reviewable

[isotoxin]

What's -2?

Just create define
#define ALMOST_DELETED_PEER -2

sent is set but never read. @isotoxin what was the intention here?

Copy-paste artefact. No need for this assignment

@isotoxin what's the difference between peer_index and peernumber?

Old irg's code used "peernumber" for two different entities: peer index and peer id.
I renamed peer id entities to gid and indices to peerindex. That's it.
group_peernumber_is_ours functions is api function. I did not rename it. But it accepts peer index.

[isotoxin]

Why was this 9 again? I keep forgetting, which means we need a named constant with an explanation as to why it's 9.

see group_packet_index
and offer your solution to how we can define 9

[iphydf]

Review status: all files reviewed at latest revision, 54 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 892 at r3 (raw file):

    }

    return (g->numpeers - 1);

Remove extra () from here and all other return value expressions.

---

toxcore/group.c, line 895 at r3 (raw file):

}

static void free_peer_stuff(Group_Peer *peer)

Choose a better name than stuff.

---

toxcore/group.c, line 909 at r3 (raw file):

 *
 * return 0 if success
 * return -1 if error.

This is a 1-bit return value => use bool.

---

toxcore/group.c, line 1226 at r3 (raw file):

}

/* Copy the public key of peernumber who is in groupnumber to pk.

Rename all peernumbers to peer_index if they are in fact peer_indices. Not all peernumbers were renamed, so I'm not sure which one should be gid and which should be peer_index.

---

toxcore/group.c, line 1375 at r3 (raw file):

 * return -3 if we are not connected to the group chat.
 */
int group_peernumber_is_ours(const Group_Chats *g_c, int groupnumber, int peer_index)

Rename this function to group_peer_index_is_ours. The public API function should stay the same, but the internal group function name can be clarified.

---

toxcore/group.c, line 1414 at r3 (raw file):

*
* return -1 on failure.
* return type on success.

It doesn't return a type, it returns 0 on success. Should it return a type? If not, make it return bool.

---

toxcore/group.c, line 1434 at r3 (raw file):

 */
static bool send_packet_group_peer(Friend_Connections *fr_c, aint friendcon_id, uint8_t packet_id,
                                   uint16_t other_group_num, const uint8_t *data, aint length)

Can length be negative? If not, it should not be a signed integer type.

---

toxcore/group.c, line 1443 at r3 (raw file):

    other_group_num = htons(other_group_num);
    uint8_t  packet[MAX_CRYPTO_DATA_SIZE];

Remove extra space.

---

toxcore/group.c, line 1456 at r3 (raw file):

 *  return 0 on failure
 */
static bool send_lossy_group_peer(Friend_Connections *fr_c, aint friendcon_id, uint8_t packet_id,

Why is friendcon_id an aint now? It's being cast back to int later in this function.

---

toxcore/group.c, line 3074 at r3 (raw file):

                }

                delpeer(g_c, groupnumber, index);

Why does every caller of delpeer ignores the return value?

---

toxcore/group.c, line 3336 at r3 (raw file):

    }

    /*

Use #if 0 to comment out blocks of code.

---

toxcore/group.c, line 3441 at r3 (raw file):

static int64_t deltatime(uint64_t t1, uint64_t t2)
{
    return (int64_t)(t1 - t2);

This can cause signed integer overflow and thus undefined behaviour.

---

toxcore/group.c, line 3449 at r3 (raw file):

    for (i = 0; i < g_c->num_chats; ++i) {

Remove all empty lines after {.

---

toxcore/group.c, line 3478 at r3 (raw file):

        }

        int next_time = 333;

What's 333?

---

toxcore/group.c, line 3487 at r3 (raw file):

                && !jd->unsubscribed) {

            next_time = 5000;

What are all these next time numbers?

---

toxcore/group.c, line 3530 at r3 (raw file):

    aint gn;
    aint jpi;

Also remove all empty lines before }.

---

toxcore/group.c, line 3598 at r3 (raw file):

static void restore_conference(Group_Chats *g_c)
{
    uint64_t ct = current_time_monotonic();

s/ct/now

---

toxcore/group.c, line 3600 at r3 (raw file):

    uint64_t ct = current_time_monotonic();

    Group_Join_Peer *jd = NULL;

Who's J.D.? What does the d stand for?

---

toxcore/group.c, line 3604 at r3 (raw file):

    Group_c *g = NULL;

    aint minnum = MAX_FAILED_JOIN_ATTEMPTS;

min_num (I guess?)

---

toxcore/group.c, line 3738 at r3 (raw file):

        set_next_join_try(g_c, jd->real_pk, ct +
                          20000); /* next try in 20 sec!!! It is important to wait for answer to this try */

Make a constant for this.

---

toxcore/group.c, line 3744 at r3 (raw file):

    }

    g->next_join_check_time = ct + 333;

Oh look, there is 333 again. Make a constant for it.

---

toxcore/group.c, line 3838 at r3 (raw file):

void do_groupchats(Group_Chats *g_c, void *userdata)
{
    unsigned i;

num_chats is uint16_t. i is only ever used to iterate over that.

---

toxcore/group.c, line 3846 at r3 (raw file):

        for (i = 0; i < g_c->num_chats; ++i) {

            Group_c *g = g_c->chats + i;

&foo[i]

---

toxcore/group.c, line 3865 at r3 (raw file):

    for (i = 0; i < g_c->num_chats; ++i) {

        Group_c *g = g_c->chats + i;

Same here.

---

toxcore/group.c, line 3958 at r3 (raw file):

    for (i = 0; i < g_c->num_chats; ++i) {

        Group_c *g = g_c->chats + i;

And here. And in other places.

---

Comments from Reviewable

[isotoxin]

Why does every caller of delpeer ignores the return value?

There are two places where delpeer called. Both of them already checked groupnumber parameter before call. delpeer can be failed only if wrong groupnumber is passed into it. So, due correct groupnumber is passed, no need to check return value.

return (int64_t)(t1 - t2);
This can cause signed integer overflow and thus undefined behaviour.

Omg! Are you serious? This is not problem for next 89 years. Even if the tox will alive after 89 years, this code will work correctly. Just try to understand how this function is used and do not offer stupid notes.
Oh. So many dubious niggles... Sorry, I just dont't understand why somebody should do this never-ending task (monkey work)

[iphydf]

If delpeer can only ever be called with the correct group number, why does it need to have a check like that at all? Seems more a job for an assert at that point.

[isotoxin]

If delpeer can only ever be called with the correct group number, why does it need to have a check like that at all? Seems more a job for an assert at that point.

Because every function must check incoming params, isn't it? Security related software... blah blah. You'd be the first person who would insist on this check

[Diadlo]

Reviewed 16 of 20 files at r25.
Review status: 16 of 17 files reviewed at latest revision, 37 unresolved discussions.

---

auto_tests/conference_test.c, line 60 at r23 (raw file):

Previously, iphydf wrote…

In fact, I'm considering not making this a requirement at all. Why do we reject double join? There are a few error conditions in toxcore that seem a bit pointless. What do you think?

I'm ok with with it if it will be declared behaviour. So remove TODO and test case?

---

toxcore/group.h, line 162 at r25 (raw file):

/* Set the callback for group invites.
 *
 *  Function(Group_Chats *g_c, int32_t friendnumber, uint8_t type, uint8_t *data, uint16_t length, void *userdata)

In real friendnumber is uint32_t

---

toxcore/group.h, line 171 at r25 (raw file):

/* Set the callback for group messages.
 *
 *  Function(Group_Chats *g_c, int groupnumber, int friendgroupnumber, uint8_t * message, uint16_t length, void *userdata)

groupnumber and friendgroupnumber is uint32_t

---

toxcore/group.h, line 179 at r25 (raw file):

/* Set callback function for title changes.
 *
 * Function(Group_Chats *g_c, int groupnumber, int friendgroupnumber, uint8_t * title, uint8_t length, void *userdata)

Same as above

---

Comments from Reviewable

[JFreegman]

Peer numbers aren't defined anywhere. I assume that they're no different from friend numbers, meaning it's not safe to just iterate 0 through max_peers and assume those are all valid peer numbers (they could be random ID's for all I know). When we join a conference, we receive the dubious event conference_peer_list_changed. Toxcore is telling us that it updated its internal list of peers. It's not telling us explicitly what changed, and it doesn't provide a way to do anything useful with this information. Therefore clients can't create an internal mapping of peer names -> peernumbers without risking UD or failed API calls.

Peernumbers need to be defined and/or provided by the API, and there should be no conference_peer_list_changed event at all. A client only needs to know three things: When a peer joins, when a peer exits, and when a peer changes their state (e.g. name). There should be three separate callbacks for each explicit event. There's no use in Toxcore telling clients arbitrary information about its internal states, and expecting clients to infer what they need to extract from those structures (much less without providing a defined method to do this).

[iphydf]

@JFreegman I agree with all that, but fixing it takes a bit of work I don't want us to do before the release. We'll live with this for now, and either fix it in the next release, or have new group chats ready enough by then so we can drop this altogether.

[iphydf]

@robinlinden there are some minor comments left on this PR. Can you take care of them?

[robinlinden]

Review status: 8 of 17 files reviewed at latest revision, 37 unresolved discussions.

---

toxcore/group.c, line 870 at r3 (raw file):

Previously, iphydf wrote…

It's unclear whether this use of memset is valid in C99. In general, it's not, but it can be, iff nick, object, and lossy are initialised separately before use. It's not locally verifiable whether that's the case.

Done.

---

toxcore/group.c, line 2228 at r3 (raw file):

Previously, iphydf wrote…

Misaligned memory access. If data is properly aligned (not locally known, as data comes from somewhere inside Messenger), data + 1 is not.

Done. (I think. This comment has been dislocated.)

---

toxcore/group.h, line 162 at r25 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

In real friendnumber is uint32_t

Done.

---

toxcore/group.h, line 171 at r25 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

groupnumber and friendgroupnumber is uint32_t

Done.

---

toxcore/group.h, line 179 at r25 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Same as above

Done.

---

Comments from Reviewable

[sudden6]

Reviewed 2 of 21 files at r22, 4 of 20 files at r25, 2 of 11 files at r26.
Review status: 9 of 17 files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 2182 at r23 (raw file):

Previously, iphydf wrote…

Not now.

ok

---

toxcore/group.c, line 2687 at r23 (raw file):

Previously, iphydf wrote…

No, if it was sent to 0 peers, we return -4 (failed to send).

ok

---

toxcore/group.c, line 2499 at r24 (raw file):

Previously, iphydf wrote…

Doxygen: https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdp. I'm slowly doxygenising the comments throughout the code in hopes of eventually being able to generate useful docs.

ok

---

toxcore/group.c, line 500 at r26 (raw file):

    } else if (new_size_of_list > g->numpeers_in_list) {
        /* Expand peers_list and put indexes of new peers into it */
        uint16_t *temp = (uint16_t *)realloc(g->peers_list, sizeof(uint16_t) * new_size_of_list);

error handling if realloc fails?

---

toxcore/group.c, line 576 at r26 (raw file):

    for (uint8_t i = 0; i < DESIRED_CLOSE_CONNECTIONS; ++i) {
        if (closest(g, i)) {

optional: invert condition and use continue, saves a layer of indentation

---

toxcore/group.c, line 1906 at r26 (raw file):

    Group_c *g = get_group_c(g_c, groupnumber);

    if (g) {

invert and use early return to match the general style

---

toxcore/group.c, line 2391 at r26 (raw file):

static bool self_peer_gid_collision(Group_c *g)
{
    uint32_t me = ~0;

use UINT32_MAX

---

Comments from Reviewable

[robinlinden]

Review status: 9 of 17 files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 576 at r26 (raw file):

Previously, sudden6 wrote…

optional: invert condition and use continue, saves a layer of indentation

Done.

---

toxcore/group.c, line 1906 at r26 (raw file):

Previously, sudden6 wrote…

invert and use early return to match the general style

Done.

---

toxcore/group.c, line 2391 at r26 (raw file):

Previously, sudden6 wrote…

use UINT32_MAX

Done.

---

Comments from Reviewable

[JFreegman]

toxcore/group.c, line 1180 at r28 (raw file):

    for (uint16_t i = groupnumber + 1; i < g_c->num_chats; ++i) {
        if (g_c->chats[i].live) {
            return 0;

When kill_groupchats() is called from tox_kill(), this function fails to free the memory associated with some of the groups (via realloc_conferences() below) because of this line. I'm not sure what the live variable indicates but all memory associated with conferences should be free'd on shutdown regardless of the state. Commenting this line out fixes this particular memory leak, but causes others elsewhere.

---

Comments from Reviewable

[iphydf]

Please put your review comments on #826. Sorry for the redundancy. This PR is too messy now.