Skip to content
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

receive buffer cleanups #1449

Closed
wants to merge 19 commits into from
Closed

receive buffer cleanups #1449

wants to merge 19 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Apr 25, 2017

Here's a series of 18 smallish commits that cleans up buffer use all over libcurl. I want to make sure the receive buffer is only used for actually receiving data, and ultimately make sure our different buffers don't all reuse BUFSIZE so that we can have the buffers shrink/grow more independently and controlled.

Changes include:

  1. Stop clobbering the receive buffer for small private sprintfs()s and move those to private stack arrays instead
  2. Make sure buffer_size is always set and represents the current buffer size length
  3. Use the buffer_size variable everywhere instead of BUFSIZE for receive buffer length since we've asked for a buffer of that size. Accessing the receive buffer beyond buffer_size should be a mistake/error.

And since the buffer is actually used to receive data and some of our code cannot handle streaming data completely, the receive buffer must have a certain size to still let libcurl function properly. One patch in this set thus sets the minimum buffer_size to be 1024.

@mention-bot
Copy link

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jrn, @dfandrich and @captain-caveman2k to be potential reviewers.

@bagder bagder force-pushed the bagder/buffer-use-cleanups branch 2 times, most recently from b24b269 to 8330532 Compare April 25, 2017 17:39
@bagder
Copy link
Member Author

bagder commented Apr 25, 2017

Test 1060 fails miserably if we allow the receive buffer to be smaller than about 16K, so I'm now considering either

  1. Use 16K as the minimum buffer size to set or
  2. Allocate and use a separate and dedicated buffer for the proxy CONNECT logic and then ditch that once the connection is made

@bagder
Copy link
Member Author

bagder commented Apr 25, 2017

It feels lame to limit the ability to set the receive buffer size because we need a buffer for CONNECT, so I'm going for alternative (2): a separately allocated and short-lived buffer for that.

@jay
Copy link
Member

jay commented Apr 26, 2017

Do you think there might be some logic issue in CONNECT that contributes to this, in other words a bug that is just separate from this issue? Say you make a separate buffer, and host foo sends an even larger amount than 1060, then maybe CONNECT would still fail?

@bagder
Copy link
Member Author

bagder commented Apr 26, 2017

Say you make a separate buffer, and host foo sends an even larger amount than 1060, then maybe CONNECT would still fail?

Yes, that's for sure. The CONNECT code is written to work with a fixed-size buffer and if the response is larger than so, it will break. So yes, that's a separate issue that should be worked on separately. But I would also expect that it is a very rare happening to see such big response to a CONNECT.

@bagder bagder force-pushed the bagder/buffer-use-cleanups branch from 8330532 to 412492e Compare April 26, 2017 06:10
Don't clobber the receive buffer.
The buffer is needed to receive FTP, HTTP CONNECT responses etc so
already at this size things risk breaking and smaller is certainly not
wise.
... instead of clobbering the download buffer.
To make it suitably independent of the receive buffer and its flexible
size.
It was a wrong assumption that it could do that!
... to properly use the dynamically set buffer size!
@bagder bagder force-pushed the bagder/buffer-use-cleanups branch from 412492e to b3caf9b Compare April 30, 2017 21:49
@bagder bagder closed this in 4858c45 May 1, 2017
@bagder bagder deleted the bagder/buffer-use-cleanups branch May 1, 2017 22:06
@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants