576902 - libssl: Avoid allocating buffers in ss->gs whenever possible

Status: NEW

(NSS :: Libraries, enhancement, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. It is possible to save 18KB per connection by merging ss->gs.buf and ss->gs.inbuf, by recv'ing the data into ss->gs.buf and then decrypting it in place. 

2. Very often, the application will be recv'ing using a buffer that is larger than a  ciphertext record. For example, an 4KB recv buffer will handle the vast majority of records received from a client in a HTTPS server and an 18KB recv buffer will handle any possible record received. By recv'ing the ciphertext fragment directly into the application's recv buffer, and then decrypting it in place, we can avoid allocating ss->gs.buf in these cases.

In addition to saving 18-36KB of RAM per connection, reducing the number of buffers used should slightly improve performance by reducing cache pressure and by (usually) avoiding one memcpy per received record.

The main problem with optimization #2 is that an application might have assumed that the contents of the buffer were not modified if no data is returned, and/or that the contents of the buffer after the returned plaintext where not modified.  So, optimization #2 may need to be controlled by an option that disables it by default. Also, optimization #2 wouldn't be safe when used in conjunction with a version of freebl/softoken that suffers from bug 576901 when using an RC4-based ciphersuite.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: incomplete, work in progress

This patch is unfinished. Things not yet done: dealing with the RC4 thread safety issue, the option for controlling optimization #2, handling PR_MSG_PEEK, and handling any SECWouldBlock that we may get after we've already returned part of the application data, and handling the case where get stumbled across some application data during a handshake (in which case we don't need to call DoRecv).

Feedback on the approach is appreciated.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch that removes ss->gs->inbuf

This patch implements optimization #1.

There are absolutely no drawbacks or gotchas to optimization #1. This patch should be easy to review. This patch also fixes bug 571795, bug 576983, bug 576984, and the important parts of bug 571732, because they were all in the way.   I will be marking my patches in those bugs obsolete and making them depend on this bug.

I am not planning to implement optimization #2 anytime soon, so I think this patch should be reviewed & checked in without it.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Improved patch to implement optimization #1.

This new patch actually removed the now-unused ss.gs->gs.inbuf structure member,  also removed the already-unused ss->saveBuf, fixed documentation, added comments to justify the way the temporary buffer for decompression is allocated and freed.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 456647 [details] [diff] [review]
Improved patch to implement optimization #1.

Brian,  Is this the self-contradiction it appears to be?

>     /* If we will be decompressing the buffer we need to decrypt somewhere
>+     * other than into ss->gs.buf. */
>     if (crSpec->decompressor) {
>+        /* We don't use sslBuffer_Grow because it always allocates a buffer 
>+         * at least 18K in size. Since this is compressed data, it is 
>+         * probably much smaller than 18K.
>+         *
>+         * Since this buffer will contain plaintext, it needs to be zeroed
>+         * before being freed.
>+         */
>+        tempBuf = PORT_Alloc(ciphertextLen);


Then, later

>     /*
>      * The decrypted data is now in plaintext.
>      */
> 
>+    /* Possibly decompress the record. If we aren't using compression then
>+     * the uncompressed data is already in ss->gs.buf. */
>     if (crSpec->decompressor) {
>+        /* The output buffer's length must be at least one *larger* than (not
>+         * just equal to) MAX_FRAGMENT_LENGTH in order for
>+         * ssl_DeflateDecompress to work correctly; luckily, ss->gs.buf is
>+         * always allocated with sslBuffer_Grow, and sslBuffer_Grow always
>+         * allocates at least MAX_FRAGMENT_LENGTH + 2048 bytes.
>+         */
>+        PORT_Assert(ss->gs.buf.space > MAX_FRAGMENT_LENGTH);
> 	rv = crSpec->decompressor(crSpec->decompressContext,
>+				  ss->gs.buf.buf,
>+                                  &plaintextLen,
>+                                  ss->gs.buf.space,
>+				  plaintextBuf,
>+				  plaintextLen);

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #4)
> Brian,  Is this the self-contradiction it appears to be?

I believe everything is correct. When we are going to decompress, the plaintext buffer holds the *compressed* plaintext in a buffer that might be less than what sslBuffer_Grow allocates. Then it gets decompressed into an output buffer that sslBuffer_Grow allocates.

Comment 6

[Nicholas Nethercote [inactive]]

Brian, Nelson:  what's the status here?  This sounds like a good bug to fix, both from the memory reduction POV and all the other bugs it fixes (listed in comment 2).  If we're waiting on Nelson's review, can somebody else do the review instead?

Comment 7

[Nicholas Nethercote [inactive]]

Another ping.  This patch has been awaiting review for almost 18 months!  And it's blocking four other bugs.

Is Nelson still working on this stuff?  bsmith, can someone else do the review?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 456647 [details] [diff] [review]
Improved patch to implement optimization #1.

AFAICT, this bug is over-prioritized for the Memshrink effort. For Firefox, we are almost definitely better off working on other things in Necko/PSM for Memshrink. My patch only saves 18KB of memory per SSL connection.

Actually, I am not unsure that it is correct, as I wrote it when I knew nothing nearly nothing about libssl. Nelson is working on other stuff so I will ask somebody else to review a new version of this patch when I have time to revise it. As for the bugs it blocks; the patches for those bugs can be refactored to not depend on this one, if necessary.

Comment 9

[Nicholas Nethercote [inactive]]

> AFAICT, this bug is over-prioritized for the Memshrink effort. For Firefox,
> we are almost definitely better off working on other things in Necko/PSM for
> Memshrink. My patch only saves 18KB of memory per SSL connection.

Which Necko/PSM things are more important?  How many SSL connections do we typically have at any one time?