Make friend requests stateless #60

GrayHatter · 2016-08-22T21:46:22Z

Messenger is slightly twisty when it comes to sending connection status callbacks
It will very likely need at the very least a partial refactor to clean it up a
bit. Toxcore shouldn't need void *userdata as deep as is currently does.

This change is

nurupo · 2016-08-23T09:15:06Z

Toxcore shouldn't need void *userdata as deep as is currently does.

We discussed it a bit with iphy on IRC. Internal layers' callbacks have their own userdata which is actually called object. Refactoring object to be a struct of two members: to what currently is being passed as object and userdata, should remove the explicit userdata propagation into the deeper layers, making them unaware of its existence. This is something to be done after we are done with making the callbacks stateless the way we currently do -- by explicitly passing userdata around. We decided to do it in steps like so.

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

iphydf · 2016-08-23T15:20:38Z

Can you do the reformat and amend dance?

GrayHatter · 2016-08-24T06:38:51Z

done

iphydf · 2016-08-24T09:07:45Z

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toxcore/friend_requests.c, line 137 [r1] (raw file):

    message[sizeof(message) - 1] = 0; /* Be sure the message is null terminated. */

    (*fr->handle_friendrequest)(fr->handle_friendrequest_object, source_pubkey, message, message_len, userdata);

This line is a bug. fr->handle_friendrequest_userdata is still set, and still assumed to be used by client code, but is no longer used. Please fix.

Comments from Reviewable

GrayHatter · 2016-08-24T09:25:00Z

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.

toxcore/friend_requests.c, line 137 [r1] (raw file):

Previously, iphydf wrote…

This line is a bug. fr->handle_friendrequest_userdata is still set, and still assumed to be used by client code, but is no longer used. Please fix.

Done.

Comments from Reviewable

iphydf · 2016-08-24T09:54:20Z

Review status: 0 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toxcore/friend_requests.c, line 137 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Done.

Ok, I'm still waiting for another change, which will follow from things you'll need to do to get this to compile.

Comments from Reviewable

iphydf · 2016-09-02T21:39:51Z

Review status: 0 of 25 files reviewed at latest revision, 2 unresolved discussions.

auto_tests/tox_test.c, line 957 [r4] (raw file):

        toxes[i] = tox_new(&opts, 0);
        ck_assert_msg(toxes[i] != 0, "Failed to create tox instances %u", i);
        tox_callback_friend_request(toxes[i], accept_friend_request, &to_comp);

Introduction of bug.

auto_tests/tox_test.c, line 1021 [r4] (raw file):

        for (i = 0; i < NUM_TOXES_TCP; ++i) {
            tox_iterate(toxes[i], NULL);

Fix bug here.

Comments from Reviewable

GrayHatter · 2016-09-02T21:56:50Z

Review status: 0 of 25 files reviewed at latest revision, 3 unresolved discussions.

auto_tests/tox_test.c, line 1021 [r4] (raw file):

Previously, iphydf wrote…

Fix bug here.

Done.

toxcore/friend_requests.c, line 137 [r1] (raw file):

Previously, iphydf wrote…

Ok, I'm still waiting for another change, which will follow from things you'll need to do to get this to compile.

Done.

Comments from Reviewable

nurupo · 2016-09-03T06:56:32Z

Reviewed 14 of 18 files at r2, 5 of 6 files at r3, 5 of 5 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

            connection_kill(c, crypt_connection_id, NULL);
            /* In this case it's safe to pass NULL back to connection_kill() because in no sane scenario will we ever
             * be already connected to our own DHT public_key */

I wonder if that's a reasonable assumption.

Comments from Reviewable

iphydf · 2016-09-03T10:28:51Z

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

toxcore/Messenger.c, line 841 [r5] (raw file):

                              void *), void *userdata)
{
    void (*handle_friendrequest)(void *, const uint8_t *, const uint8_t *, size_t, void *) = (void *)function;

This cast was invalid, but now the next line is a warning. Please add a valid cast to the function call.

Comments from Reviewable

iphydf · 2016-09-03T11:19:14Z

Review status: 24 of 28 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

auto_tests/toxav_basic_test.c, line 162 [r7] (raw file):

    uint8_t address[TOX_ADDRESS_SIZE];

    tox_callback_friend_request(Alice, t_accept_friend_request_cb, &to_compare);

Have you ensured that to_compare is either unused or passed to tox_iterate?

Comments from Reviewable

GrayHatter · 2016-09-05T09:29:34Z

Review status: 23 of 28 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

auto_tests/toxav_basic_test.c, line 162 [r7] (raw file):

Previously, iphydf wrote…

Have you ensured that to_compare is either unused or passed to tox_iterate?

`to_compare` is ignored in this case.

Considering this is the toxav test. I'm okay with ignoring this check in this test. The check should be done in tox_test.c. Which it is (sorta).

toxcore/Messenger.c, line 841 [r5] (raw file):

Previously, iphydf wrote…

This cast was invalid, but now the next line is a warning. Please add a valid cast to the function call.

Done.

Comments from Reviewable

GrayHatter · 2016-09-05T09:33:15Z

Review status: 23 of 28 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

Previously, nurupo wrote…

I wonder if that's a reasonable assumption.

I haven't given it a TON of thought, but I cant' think of a case where it would be okay to be connected to our current dht key.

Comments from Reviewable

iphydf · 2016-09-05T10:33:37Z

Reviewed 12 of 18 files at r2, 4 of 6 files at r3, 5 of 5 files at r4, 1 of 1 files at r5, 3 of 4 files at r6, 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

GrayHatter · 2016-09-05T10:44:48Z

Do not merge until someone has verified that the TCP callbacks are getting called correctly!

Do no merge until someone is able to verify the tests for the connection status are working as expected!

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

GrayHatter · 2016-09-05T10:49:40Z

Okay, confirmed the tests, and the TCP callbacks.

This can be merged with one more LGTM

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

GrayHatter · 2016-09-05T10:51:58Z

Ignore the branch name, this pull makes friend_requests callbacks stateless, not friend_connections.

Friend connections had to be updated to make sure that iterate() is able to pass up userdata to the friend_request callback.

Friend connection callbacks should be next on the list for callbacks.

nurupo · 2016-09-05T20:29:33Z

Reviewed 3 of 4 files at r6, 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

I haven't given it a TON of thought, but I cant' think of a case where it would be okay to be connected to our current dht key.

I still don't get why you think that we will be connected to our own DHT pubkey inside that if-statement. How does `n_c.dht_public_key` and `conn->dht_public_key` being different keys imply that? Because that's the only case when the if-condition is true.

Comments from Reviewable

GrayHatter · 2016-09-05T23:27:17Z

Review status: 27 of 28 files reviewed at latest revision, 1 unresolved discussion.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

Previously, nurupo wrote…

I still don't get why you think that we will be connected to our own DHT pubkey inside that if-statement. How does n_c.dht_public_key and
conn->dht_public_key being different keys imply that? Because that's the only case when the if-condition is true.

We shouldn't ever be connected to our own DHT pubkey.

This is here from a change @iphydf asked for, I didn't want this to get missed, so I added this comment.

Comments from Reviewable

nurupo · 2016-09-05T23:36:57Z

Review status: 27 of 28 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

We shouldn't ever be connected to our own DHT pubkey.

This is here from a change @iphydf asked for, I didn't want this to get missed, so I added this comment.

I'm confused. The comment clearly states "we are connected to our own DHT pubkey".

Comments from Reviewable

GrayHatter · 2016-09-05T23:49:33Z

Review status: 26 of 28 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toxcore/net_crypto.c, line 1760 [r5] (raw file):

Previously, nurupo wrote…

I'm confused. The comment clearly states "we are connected to our own DHT pubkey".

Done.

Comments from Reviewable

nurupo · 2016-09-06T00:05:02Z

Review status: 26 of 28 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

nurupo · 2016-09-06T00:44:57Z

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Messenger is slightly twisty when it comes to sending connection status callbacks It will very likely need at the very least a partial refactor to clean it up a bit. Toxcore shouldn't need void *userdata as deep as is currently does. (amend 1) Because of the nature of toxcore connection callbacks, I decided to change this commit from statelessness for connections changes to statelessness for friend requests. It's simpler this was and doesn't include doing anything foolish in the time between commits. group fixup because grayhatter doesn't want to do it "arguably correct" is not how you write security sensitive code Clear a compiler warning about types within a function.

iphydf · 2016-09-06T09:57:03Z

Review status: 22 of 28 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

GrayHatter assigned nurupo, tux3 and iphydf Aug 22, 2016

GrayHatter force-pushed the stateless_connstat branch 2 times, most recently from 5dee222 to b7a1d01 Compare August 24, 2016 06:38

GrayHatter force-pushed the stateless_connstat branch from b7a1d01 to 288f1d2 Compare August 24, 2016 09:24

GrayHatter force-pushed the stateless_connstat branch from 288f1d2 to 0bd1908 Compare August 24, 2016 09:46

GrayHatter force-pushed the stateless_connstat branch from 0bd1908 to 592bfa7 Compare September 2, 2016 21:17

GrayHatter force-pushed the stateless_connstat branch from 1d948ab to ea7f449 Compare September 2, 2016 21:57

GrayHatter force-pushed the stateless_connstat branch from ca44f29 to 56c7a83 Compare September 5, 2016 09:40

GrayHatter changed the title ~~Make file connection stateless~~ Make friend requests stateless Sep 5, 2016

GrayHatter force-pushed the stateless_connstat branch from 56c7a83 to daab88d Compare September 5, 2016 11:10

GrayHatter force-pushed the stateless_connstat branch from daab88d to 262cbf8 Compare September 5, 2016 23:23

GrayHatter force-pushed the stateless_connstat branch from 262cbf8 to b72e530 Compare September 5, 2016 23:49

iphydf unassigned tux3 Sep 6, 2016

GrayHatter force-pushed the stateless_connstat branch 2 times, most recently from d15efc2 to 8e67a4c Compare September 6, 2016 01:51

GrayHatter force-pushed the stateless_connstat branch from 8e67a4c to aad1e0a Compare September 6, 2016 09:28

iphydf merged commit aad1e0a into TokTok:master Sep 6, 2016

iphydf modified the milestone: v0.0.1 Nov 6, 2016

GrayHatter deleted the stateless_connstat branch November 19, 2016 07:18

iacore pushed a commit to iacore/c-toxcore that referenced this pull request Jul 14, 2024

cross build on more architectures (TokTok#60)

987d6f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make friend requests stateless #60

Make friend requests stateless #60

GrayHatter commented Aug 22, 2016 •

edited by iphydf

Loading

nurupo commented Aug 23, 2016

iphydf commented Aug 23, 2016

GrayHatter commented Aug 24, 2016

iphydf commented Aug 24, 2016

GrayHatter commented Aug 24, 2016

iphydf commented Aug 24, 2016

iphydf commented Sep 2, 2016

GrayHatter commented Sep 2, 2016

nurupo commented Sep 3, 2016

iphydf commented Sep 3, 2016

iphydf commented Sep 3, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

iphydf commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 6, 2016

nurupo commented Sep 6, 2016

iphydf commented Sep 6, 2016

Make friend requests stateless #60

Make friend requests stateless #60

Conversation

GrayHatter commented Aug 22, 2016 • edited by iphydf Loading

nurupo commented Aug 23, 2016

iphydf commented Aug 23, 2016

GrayHatter commented Aug 24, 2016

iphydf commented Aug 24, 2016

GrayHatter commented Aug 24, 2016

iphydf commented Aug 24, 2016

iphydf commented Sep 2, 2016

GrayHatter commented Sep 2, 2016

nurupo commented Sep 3, 2016

iphydf commented Sep 3, 2016

iphydf commented Sep 3, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

iphydf commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 5, 2016

GrayHatter commented Sep 5, 2016

nurupo commented Sep 6, 2016

nurupo commented Sep 6, 2016

iphydf commented Sep 6, 2016

GrayHatter commented Aug 22, 2016 •

edited by iphydf

Loading