Skip to content

Conversation

@alexbakker
Copy link
Contributor

Progress:

  • SOCKS5
  • HTTP

Figured I'd submit this in advance so you guys can take a look and scrutinize my horrid C skills if needed.


This change is Review on Reviewable

@GrayHatter
Copy link
Collaborator

Reviewed 3 of 5 files at r1.
Review status: 3 of 5 files reviewed at latest revision, 4 unresolved discussions.


toxcore/TCP_client.c, line 142 [r1] (raw file):
what if the password is blank?


toxcore/TCP_client.c, line 198 [r1] (raw file):
what happens if a client forgets to use a null term?


toxcore/TCP_client.c, line 987 [r1] (raw file):
no callback to the client that the connection was refused?


toxcore/tox.c, line 198 [r1] (raw file):
what if there's no null term?


Comments from the review on Reviewable.io

@alexbakker
Copy link
Contributor Author

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


toxcore/TCP_client.c, line 142 [r1] (raw file):
In that case you'd set proxy_password to an empty string.


toxcore/TCP_client.c, line 198 [r1] (raw file):
That's their problem. C strings should always be NULL-terminated and the documentation even states that it should be.


toxcore/TCP_client.c, line 987 [r1] (raw file):
Indeed. Look at the surrounding code, you'll see the exact same thing everywhere.


toxcore/tox.c, line 198 [r1] (raw file):
See other comment.


Comments from the review on Reviewable.io

@nurupo
Copy link
Contributor

nurupo commented Mar 13, 2016

What about tox.h version tick?

I guess the version will be ticked manually by @irungentoo whenever he feels like releasing new version.

@GrayHatter
Copy link
Collaborator

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


toxcore/TCP_client.c, line 142 [r1] (raw file):
do you want to document this?


toxcore/TCP_client.c, line 987 [r1] (raw file):
Network error vs input error, we were able to connect, the password was just refused.

Also it's wrong now isn't a reason to leave it wrong. If I'm going to implement this in µTox I need to know if the server rejected the connection, or the auth.


Comments from the review on Reviewable.io

@alexbakker
Copy link
Contributor Author

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion.


toxcore/TCP_client.c, line 142 [r1] (raw file):
Actually, I took another look at the SOCKS5 spec and it turns out both the username and the password must have a minimum length of 1. I added checks and comments for this.


toxcore/TCP_client.c, line 987 [r1] (raw file):
I agree. I'll see what I can do.


Comments from the review on Reviewable.io

@GrayHatter
Copy link
Collaborator

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


other/apidsl/tox.in.h, line 604 [r2] (raw file):
length


Comments from the review on Reviewable.io

@alexbakker
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


other/apidsl/tox.in.h, line 604 [r2] (raw file):
Fixed.


Comments from the review on Reviewable.io

@GrayHatter
Copy link
Collaborator

Reviewed 1 of 2 files at r3.
Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@GrayHatter
Copy link
Collaborator

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

static void proxy_socks5_generate_auth_request(TCP_Client_Connection *TCP_conn)
{
uint8_t username_len = strlen(TCP_conn->proxy_info.username);
uint8_t password_len = strlen(TCP_conn->proxy_info.password);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use string functions in tox.c but not here.

Save the length in the proxy settings struct.

@alexbakker
Copy link
Contributor Author

@irungentoo Fixed.

EDIT: I lied, sorry. This is what happens when I don't test things.
EDIT2: Should be actually fixed now. Will test when I get home.

@GrayHatter
Copy link
Collaborator

@Impyy reopen on toktok/toxcore?

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.

4 participants