dynbuf: Remove 64MB max limit on pause buffers #5966
Conversation
- Add DYN_NO_LIMIT to represent no size limit for dyn buffer. - Define DYN_PAUSE_BUFFER as DYN_NO_LIMIT. Prior to this change the pause buffer limit was 64MB, but it's possible the transfer may be paused for a long enough period of time to accumulate that amount of data. The limit was introduced when the dynbuf API was added in ed35d65 and prior to that change there was no limit. Closes #xxxx
|
First: I think we should have all dynbuf buffers bounded to some limit to avoid problems with unbounded memory reallocs. In my view, that is part of the general benefit of using dynbuf everywhere: we avoid boundless buffer growth in case of mistakes or malicious (ab)use. If 64MB is a problem, then maybe make it 512 or 2048MB ? I cannot see the case for allowing completely unbounded growth. (and probably also make sure we output a sensible error log in case we run into the limit) Then: this pause buffering really is at least annoying and sometimes even problematic to users. I know for a fact that (many?) users consider already 64MB to be completely unacceptably large amount of memory to just alloc for this purpose, and at the same time it is really tricky for a user of libcurl to know how much memory that will be used and what they can do to reduce it. But this is a much larger issue. We should probably see what we can do about documenting this pause buffer better to make it less of a surprise let users take that into account when they design their libcurl use. |
I disagree in this case and to a somewhat lesser extent in file2string which was recently changed. Arbitrary size limits unnecessarily limit the user. Perhaps the memory use warning in pause could be improved. The case for unbounded growth is we can't predict the conditions of the paused transfer. It is conceivable to me that a connection may be paused for a period of time that a large amount of data may be cached. If they have the memory for it why stop them. |
As does security measurements in any environment. We need to weigh the pros and cons and I think given a large enough limit, we can balance the restrictions with the value.
If they do it willingly, sure. I've talked to users who had the memory but certainly did not want or intend for curl to spend it on its pause buffer. As with most of our features and functions, I'm sure we can find users in both/all camps... |
|
I'm not disagreeing with your position on security I just don't think it applies here. A large amount of memory in this case is not a mistake or abuse. |
|
... and by always having a limit we make sure of that. |
|
Come to think of it: we've limited the HTTP/2 window to 32MB. Can you think of a scenario where this buffer can actually and legitimately grow larger than 64MB? |
I forgot about that. Ref: #4939 |
In retrospect I think I overstated and it's better to wait for a report before pursuing this further. |
Add DYN_NO_LIMIT to represent no size limit for dyn buffer.
Define DYN_PAUSE_BUFFER as DYN_NO_LIMIT.
Prior to this change the pause buffer limit was 64MB, but it's possible
the transfer may be paused for a long enough period of time to
accumulate that amount of data. The limit was introduced when the dynbuf
API was added in ed35d65 and prior to that change there was no limit.
Closes #xxxx