Menu

#1450 Schannel internal buffers grow without limit during large download

closed-fixed
nobody
None
5
2015-03-15
2014-11-10
No

libcurl version: 7.39.0
Operating System: Windows 7

I've been seeing an issue in the schannel code where the internal encrypted buffer keeps growing over time during a large download. I went back and saw that there was some discussion on this topic a couple of years ago, and changes were made and then later reverted. What I'm seeing looks to me like a bug in the logic, though perhaps I'm missing something.

The schannel_recv function has this code to increase the encrypted buffer when needed (there's a similar bit of code in the handshake, but I'm not seeing an issue there):

while(connssl->encdata_length - connssl->encdata_offset < CURL_SCHANNEL_BUFFER_FREE_SIZE || connssl->encdata_length < len) { / increase internal encrypted data buffer / }

My understanding is that the buffer is increased in size if either the existing buffer has less than CURL_SCHANNEL_BUFFER_FREE_SIZE (1024) bytes available in it, or if the entire existing buffer is too small to hold the requested amount of data. If either is true, the buffer is realloc'd twice its previous size.

The basic workflow in this method is:
1. Read data from the socket - enough to fill our encrypted buffer
2. While we have more encrypted data to process, and we haven't obtained enough decrypted data yet:
* Decrypt the data (as much as schannel will decrypt)
* Copy the decrypted data to the decrypted buffer
* Move any remaining encrypted data to the front of the encrypted buffer
3. Copy off the decrypted bytes to the caller's buffer
4. If there is any remaining decrypted bytes, move those to the front of the decrypted buffer (save them for the next call)

The issue seems to be that we can exit the while loop with the encrypted buffer nearly full. Then the next time the function is called, there is less than CURL_SCHANNEL_BUFFER_FREE_SIZE bytes available, and the buffer is reallocated. However, there's plenty of data in the encrypted buffer - more than enough to provide the caller with the number of bytes they requested. Let me give an example:

Say the encrypted buffer is already 32KB in size based on previous calls to this function, and the decrypted buffer contains 15.5KB of data left over from the previous call. This function is called again with a request for 16KB of data. We read enough data from the socket to fill up the encrypted buffer, and then decrypt. Say that the DecryptMessage function only returns 500 bytes of decrypted data, with the other 31.5KB left for the next decryption call. These 500 bytes, plus the 15.5KB in the decrypted buffer from last time, are enough to satisfy the caller's request, so we exit the while loop, leaving 31.5KB in the encrypted buffer. Then the next time the function is called, we are within the "free size" limit and the buffer gets reallocated. But this reallocation isn't necessary, as there's much more encrypted data available than what the caller is asking for (16KB). We should be able to read a small amount of data from the socket to fill up our decrypted buffer (500 bytes) and proceed as normal (or even skip the socket read completely since there's plenty of data already).

So I'm not clear what the "free size" check is intended to do - perhaps just make sure the buffer expands nicely from its original size? Once the buffer is larger than the len parameter, I don't believe expanding it further does any good - it just consumes extra memory. This scenario above can happen multiple times during a single download, and can result in the encrypted buffer doubling in size many times - I saw it get as high as 256 MB.

Discussion

  • Marc

    Marc - 2014-12-14

    Thanks for reporting this issue. I think you are correct about the curring buffer approach consuming to much memory. I will try to reduce the memory footprint and further optimize the memory allocation strategy.

     
  • Marc

    Marc - 2014-12-14

    Hey Warren,

    I just posted the following patch to the Github repository:
    https://github.com/bagder/curl/commit/212e3e26bc711c4bd2e0a1c2bbc14b4e40506917

    Please test if this solves the increasing memory issue for you.

    Best regards,
    Marc

     
  • Warren Menzer

    Warren Menzer - 2014-12-15

    Thanks, Marc. It's going to be a few days before I have a chance to try this patch in my code, but looking at it, I'm not sure if it completely solves the issue or not. The if statement (which is no longer a while loop) still checks:

    connssl->encdata_length - connssl->encdata_offset < CURL_SCHANNEL_BUFFER_FREE_SIZE

    I'm not sure if that's really a valid check or not. In my example above, the previous call to this method left the encrypted buffer nearly full (with less than CURL_SCHANNEL_BUFFER_FREE_SIZE bytes free in it), but depending on the length requested (len), that doesn't necessarily mean a larger buffer is needed. So while it won't double the size of the buffer each time that condition is met (which is good), it's still going to reallocate, even if there is way more encrypted data to decrypt than the caller needs.

    Does that make sense? It looks like this will get rid of the creeping memory usage - I just wonder if we'll be needlessly reallocating sometimes. It shouldn't be too common, but it would happen any time the encrypted buffer happened to be left nearly full the last time through.

     
  • Marc

    Marc - 2014-12-15

    Thanks for the feedback, Warren. The reasoning for CURL_SCHANNEL_BUFFER_FREE_SIZE is that the encrypted and decrypted data lengths do not necessarily align. For example, the encrypted buffer can be smaller (e.g. compression) or larger (e.g. padding) than the decrypted buffer. And SChannel is only able to decrypt complete SSL/TLS fragments/messages, so growing the encrypted data buffer is necessary if the requested length is not sufficient for SChannel to decrypt the fragment/message.

    I understand and also think that this memory strategy can and needs to be improved, but I don't want to completely remove that check there just yet. I ran in a lot of trouble while developing the SChannel backend because of this. While this is still not ideal, it works for now.

     
  • Warren Menzer

    Warren Menzer - 2015-01-09

    The changes work well - the lack of that multiplicative factor makes a big difference. Thanks for looking into this!

     
  • Daniel Stenberg

    Daniel Stenberg - 2015-01-11

    Good enough to consider this issue fixed, you think?

     
  • Warren Menzer

    Warren Menzer - 2015-01-12

    Yes, with the caveat I mentioned earlier that the new code results in more realloc calls, since we are hitting the "not enough room" condition more often now that the buffer is increased in size linearly rather than by doubling. In my tests, we'd start with a 16 KB buffer and increase it up to something like 128 KB by increasing the size by 1 KB at a time, so something like 100 reallocates over the course of the download. That's not a huge deal, of course, and the very large memory usage pre-fix is no longer an issue, so I'm happy with the fix.

     
  • Daniel Stenberg

    Daniel Stenberg - 2015-03-15

    Thanks, as nobody else seems to have any major complaints we consider this closed even though it may have room for future further improvements.

     
  • Daniel Stenberg

    Daniel Stenberg - 2015-03-15
    • status: open --> closed-fixed