-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
socks using its own buffer instead of data->state.buffer #12788
Conversation
That's a generated file... 😬 |
lib/socks.c
Outdated
@@ -74,6 +74,7 @@ enum connect_t { | |||
struct socks_state { | |||
enum connect_t state; | |||
ssize_t outstanding; /* send this many bytes more */ | |||
unsigned char buffer[8*1024]; /* more than enough */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you pick the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a crystal skull next to my dropping candles here in my lair.
As I read the code, one socks packets always seem to be smaller than 1k, but I might be wrong...therefore the assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started replacing the fixed buffer with a bufq, but that quickly escalated all over the file and I was not sure if it would be worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe username, password and hostname are all limited to 256 bytes max, so 1K should probably be enough.
Line 434 in the original, 435 in your update, references |
remove generated file
Nice catch! Push a fix. |
line 288: DEBUGASSERT(READBUFFER_MIN >= 600); This check verifies the the smallest download buffer is big enough. It should now probably check that |
I added a #define for the size and a compile time check. |
I did #12797 to help us verify that a ~600 byte buffer will work fine, but we can also adjust the size after the initial merge. |
No description provided.