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
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 25, 2017

@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.

mention-bot commented Apr 25, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 25, 2017

Member

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
Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 25, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 26, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 26, 2017

Member

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.

Member

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 added some commits Apr 24, 2017

http: use private user:password output buffer
Don't clobber the receive buffer.
CURLOPT_BUFFERSIZE: 1024 bytes is now the minimum size
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.
file: use private buffer for C-L output
... instead of clobbering the download buffer.
http-proxy: use a dedicated CONNECT response buffer
To make it suitably independent of the receive buffer and its flexible
size.
unit1606: do not print/access buffer
It was a wrong assumption that it could do that!
buffer: use data->set.buffer_size instead of BUFSIZE
... to properly use the dynamically set buffer size!

@bagder bagder closed this in 4858c45 May 1, 2017

@bagder bagder deleted the bagder/buffer-use-cleanups branch May 1, 2017

@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.