283763 - memsets in libssl affect performance ; SSL buffer also starts too small

Status: NEW

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

Performance profile data shows that several cleanup calls memset in libssl
affect performance significantly.
We may want to retain these memset calls for security purposes. However, when
it's time for running benchmarks, these calls should be removed . We need to
have a way to run the code less securely without them. This should probably be a
runtime setting ...

Another observation is that we call PORT_Realloc twice for really small amounts
(from 79 to 86 to 150 bytes) in sslBuffer_Grow . We should increase the default
minimum size to avoid those calls.

I will attach a patch that addresses both of these issues.

Comment 1

[Julien Pierre]

Attachment: remove cleanup memset calls, and increase default SSL buffer size

Not meant to be checked in as is; the memset need a dynamic setting.

Comment 2

[Wan-Teh Chang]

Comment on attachment 175623 [details] [diff] [review]
remove cleanup memset calls, and increase default SSL buffer size

>+    newLen = PR_MAX(newLen, 18432);

Would be nice to define a macro for the magic
number 18432.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175623 [details] [diff] [review]
remove cleanup memset calls, and increase default SSL buffer size

Julien,  Your patch removes two memsets, believing that they are only for
"security" and not necessary for correctness.  But one of them is needed
for correctness.  

Both of these memsets clear structures that were separately malloc'ed at 
one time, but are now parts of the larger SSLSocket structure.	That change 
was made to reduce malloc calls, which as you know is something we want 
to do MORE of.	These memsets took the place of PORT_ZFree calls that 
preceeded that change.	

The second of these two removed memsets only occurs when the socket is 
being closed, and is only to prevent sensitive info from remaining in 
memory after close.  But the first is used in a function that may be 
called repeatedly on an SSLSocket that is in use.  It is called from 
SSL_ResetHandshake, and removing that memset stops SSL_ResetHandshake 
from working properly.	When called from SSL_ResetHandshake, 
the first memset must be done.

The first of the two modified functions is also called from the second, 
which means that the first memset is overlapped by the second one.  
I propose adding an argument to the first function, which is internal,
that tells it whether or not to do that memset.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175623 [details] [diff] [review]
remove cleanup memset calls, and increase default SSL buffer size

BTW, the comment just above function ssl_DestroySecurityInfo is 
incorrect.  It is not called from SSL_ResetHandshake.  It is the
previous function that is called from SSL_ResetHandshake.
So, when this bug is fixed, that comment should be fixed too.

> /*
> ** Called from SSL_ResetHandshake (above), and 
> **        from ssl_FreeSocket     in sslsock.c

Comment 5

[Julien Pierre]

Attachment: use macro for buffer size, and conditionnally do the first memset

still not for checkin

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This work is planned for 3.11

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.