305147 - Enhance SSL performance, especially for servers

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSS's SSL performance needs improvement.  
It is slower than many other implementations.
There are numerous reasons for that, including:
- overhead of PKCS11 is very high for very small operations, such as 
  RC4 encryption of (say) 25 bytes, and adds lock contention to operations
  that are inherently share no resources except PKCS11 internal object lists.
- wrapping and unwrapping of master secrets for storage in session cache
- locking and unlocking of SSL socket objects, some of which is defensive.
- unwrapping "token" private keys for each and every use.
- insufficiently optimized cryptographic algorithms 

Some of those are the subjects of other bugs.  
This bug is about the PKCS11 overhead.
I am working to reduce/eliminate most of it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm shortly going to attach some large patches to bug XXXXXXXX,
but first here is an overview of the changes.

1. Two new source files:

   derive.c contains two new functions used when "bypassing" PKCS11: 
     - one to derive the master secret from the premaster secret,
     - one to derive the bulk data encryption and MAC keys, and IVs.
   These new functions call blapi functions directly.

   ssl3ecc.c contains most of the ECC-related code formerly found in 
   ssl3con.c.  Most of that code was factored out of enormous switch
   statements into new functions that are now called from those switches.
   The ECC code builds, but is otherwise untested.

   One consequence of this is that numerous functions in ssl3con.c are
   no longer static.  They are declared in sslimpl.h so that they may 
   be called from other ssl3 source code.  This is a prelude to breaking
   ssl3con.c down into numerous smaller source files.

2. Changes to the sslSocket structure.

2a) Changed the sslSocket's ssl3 member from a pointer to an instance.
   Consequently, all the places that formerly referenced 
   ss->ssl3->something or ssl3->something now reference ss->ssl3.something.  
   That was the project known as "turning arrows into dots".

   The ssl3 structure contains a new member named "initialized". 
   All the code that formerly tested ss->ssl3 != NULL now tests 
   ss->ssl3.initialized.

2b) Moved all the SSL socket option bits into a new structure, an instance
   of which is part of the SSL socket struct.  All the code that formerly 
   referenced the bits directly (e.g. ss->enableTLS) now references them
   in the options struct (e.g.  ss->opt.enableTLS).  This enables all 
   the option bits to be copied from the global defaults, or from another
   SSL socket to a new SSL socket in a single load/store, rather than 
   each bit having to be copied individually.  

   These two changes affected MANY lines of code, but the changes are 
   all mechanical and easy to spot while reviewing.  In many files,
   these changes were the only changes to those files.

3. SSL sockets have two new socket options that may be set on individual
   sockets, or set in global defaults.  One enables the "bypass" feature.
   The other disables locks in the SSL socket used to protect the SSL
   socket's contents from multiple threads.  The default settings for 
   both of these features can be controlled through environment variables,
   and default to disabled in the absence of those variables.

4. Quite a few of the larger functions in ssl3con.c have been factored
   into a number of smaller functions.  For example: 
   ssl3_InitPendingCipherSpec is now a MUCH smaller function, which calls
   ssl3_DeriveMasterSecret to derive the MS from the PMS, then calls
   either  ssl3_KeyAndMacDeriveBypass and ssl3_InitPendingContextsBypass
   or ssl3_DeriveConnectionKeysPKCS11 and ssl3_InitPendingContextsPKCS11.

   The code that wraps a master secret to put it into the cache has been
   factored out of ssl3_HandleFinished into a new function named
   ssl3_CacheWrappedMasterSecret.  This reduces indentation, makes the
   functions shorter and easier to follow.  

5. Pairs of certs and private-keys are now held in a reference-countable
   struct.  This reduces key copying overhead for each new accepted 
   server socket.

   SSL_ConfigSecureServer, which adds a cert and private key pair to a
   a server socket, now attempts to copy the token private key to a 
   session private key, and use the latter.  This has significant 
   performance benefits when used with Softoken.  

6. The "bypass" feature, which performs some cryptographic computations by
   calling the freebl functions directly through the blapi API.  
   The single biggest effect of this change is an increase in the size of 
   the sslSocket structure, which now contains the symmetric crypto and 
   MAC contexts, and associated key material. 

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: changes to SSL header files

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Two new source files

These are derive.c and ssl3ecc.c, as unified diffs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: The monster patch

There may be one more attachment to be made, containing prerequisite 
changes to other files (outside of nss/lib/ssl).

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194279 [details] [diff] [review]
changes to SSL header files

Bob, I'm asking you for review, but you and Wan-Teh can decide who should
review these 3 patches. 
I'm only putting the review request on the first of the patches, but please
review them all as a set. 

There is an additional patch coming that adds some new command line options to
tstclnt, strsclnt, selfserv and ssl.sh, to test these new features.  However, I
don't think the review of these changes to nss/lib/ssl needs to wait for that
additional patch.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194281 [details] [diff] [review]
The monster patch

Bob, Please advise on this issue.

>     if (key) {
>-	sc->serverKey = SECKEY_CopyPrivateKey(key);
>-	if (sc->serverKey == NULL)
>+	/* should call pk11_mapSignKeyType(key->keyType) here XXX */
>+	CK_MECHANISM_TYPE keyMech = CKM_RSA_PKCS;
>+	/* Maybe should be bestSlotMultiple? */
>+	PK11SlotInfo * bestSlot = PK11_GetBestSlot(keyMech, NULL /* wincx */);

Do you think we should export pk11_mapSignKeyType from libNSS3 
for this purpose?  (Perhaps renamed to PK11_MapSignKeyType)

>     /* TLS MAC includes the record's version field, SSL's doesn't.
>-    ** We decide which MAC defintion to use based on the version of 
>+    ** We decide which MAC defintiion to use based on the version of 

I promise to fix that spelling. :)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194279 [details] [diff] [review]
changes to SSL header files

Bob and/or Wan-Teh, please reivew for upcoming 3.11 alpha.  
There are 3 parts to this patch, separately attached to this bug.  Please
review all 3 parts.  Thanks.

Comment 8

[Robert Relyea]

Comment on attachment 194279 [details] [diff] [review]
changes to SSL header files

I've reviewed this patch, and the mondo patch. I have lots of comments on the
latter (some of which really need to be addressed), but I don't see any
structural problems. I think they are better managed by checking in this patch
and evaluating the additional changes as patches on top of this one.

bob

Comment 9

[Robert Relyea]

Comment on attachment 194281 [details] [diff] [review]
The monster patch

I hanven't reviewed the new files yet, though they are clearly required to
check in.

Comment 10

[Robert Relyea]

I'm emailing my comments to nelson because they are quite wordy. The short
version of the issues are:

Main issues:
1) I think this version of RC2-40 will not interoperate with anyone else (I
still could be wrong on this one - but my tests seem to indicate otherwise).
2) Some inconsistancies in the restart code between SendClientHello,
HandleServerHello, SendServerHello.
3) Answer to Nelson's question comment 6: Yes, we should export a public version
of this function (with the public name) and use it here. right now if you happen
to have a token installed that can only do RSA and it's set as your default RSA
device, your DH key won't get 'sessionized'.
[plus some piddly things that still should get fixed].

Main non-critical issues:
1) Worry about HW performance in the new ssl_SendRecord due to turning one
encrypt call into 2.
[more piddle little things that aren't as important to get fixed]

Comment 11

[Robert Relyea]

Comment on attachment 194280 [details] [diff] [review]
Two new source files

OK to bring this in. My only comments are generalization of the same issues I
raised in my comment on the monster  (piddly issues about making the SSL3
macing and key gen it's own function & the idea of trying to reduce the
redundancy betwen ECDH and DH by combining the functions).

Neither are critical, nor require addressing to checking or even ship 3.11

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Changes to files outside of lib/ssl

This patch is new, and does not replace any previous patch.
It renames pk11_mapSignKeyType to PK11_MapSignKeyType, and moves its 
declaration from pk11wrap/secmodi.h to pk11wrap/pk11pub.h, and adds
the name to nss/nss.def.  Finally it changes existing uses of the 
old name to the new name.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Two new files (rev 2)

This is a replacement patch for the old "Two new source files" patch.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Two new source files (v3)

Perhaps interdiff will be able to diff this patch against the original
"two new source files" patch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: changes to SSL header files (v2)

This patch should compare to the original "changes to SSL header files" patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194279 [details] [diff] [review]
changes to SSL header files

removing review request for obsolete patch.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch (v2)

This file should compare to the previous "monster patch".

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Attachment: monster patch (v3)

Interdiff was unable to diff monster patch v1 with monster patch v2.
Hopefully it will be able to diff v1 with this v3.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to ssl makefiles

The "monster patch" v1 omitted these makefile changes. So these need to 
be reviewed for the first time.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: monster patch (v4)

This is one more try at producing a new version of the "monster patch"
that interdiff will successfully diff with the first "monster patch"

Comment 21

[Saul Edwards]

Attachment: Add -B (bypass on) and -s (disable SSL locking) to cmds; test with ssl.sh

Comment 22

[Robert Relyea]

Comment on attachment 195228 [details] [diff] [review]
Changes to files outside of lib/ssl

r=relyea

Comment 23

[Robert Relyea]

Comment on attachment 195237 [details] [diff] [review]
monster patch (v4)

r=relyea -- addresses all the critical issues in from my review.

Comment 24

[Robert Relyea]

Comment on attachment 195236 [details] [diff] [review]
patch to ssl makefiles

r=relyea,

one question: is there a reason we include two copies of the identical code
block in both the windows and non-windows  portion of the conditional rather
than just putting the whole thing outside that if block?

Comment 25

[Robert Relyea]

I'm sure that last comment was clear as mud... I'm talking about the code in
config.mk which adds freebl to the list of extralibs.

Comment 26

[Robert Relyea]

Comment on attachment 195231 [details] [diff] [review]
changes to SSL header files (v2)

r=relyea

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

After talking to the BMO folks, we have a working hypothesis about why 
Interdiff is unable to diff the original "monster patch" with the latest ones.
It's because ONE of the files in that patch has been changed on the trunk
between the first patch and the second, namely sslinfo.c.  

Backing that change out won't solve it.  However, I *MAY* be able to produce 
another patch which is the same as the original "monster patch", but is 
diffed from the current trunk.  I will try to produce that patch today, to 
facilitate diffing the old and new monster patches.  

I'll send out a reminder to the NSS developer mailing list, reminding about
the "car pool" on nss/lib/ssl.  

Comment 28

[Robert Relyea]

Comment on attachment 195230 [details] [diff] [review]
Two new source files (v3)

I see no diffeences between this and the original 2 new source files.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #24)
> one question: is there a reason we include two copies of the identical code
> block in both the windows and non-windows  portion of the conditional rather
> than just putting the whole thing outside that if block?  [in config.mk]

Thanks for catching that Bob.  Reviews are *so* valuable.
They were intended to be different.  Dunno why they're not.  Will investigate.  

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #28)
> (From update of attachment 195230 [details] [diff] [review] [edit])
> I see no diffeences between this and the original 2 new source files.

Yeah, I suspect this is another interdiff failure.  I'll diff the two 
patches by hand just to see.

Comment 31

[Robert Relyea]

Comment on attachment 195240 [details] [diff] [review]
Add -B (bypass on) and -s (disable SSL locking) to cmds; test with ssl.sh

r=relyea

Comment 32

[Robert Relyea]

I think these are good to go...

bob

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Thanks Bob.  We're going to do a little more testing before the checkin.
So there might be another patch or two later today or tomorrow.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

I checked in the non-lib/SSL changes that are prerequsites for the new SSL code.

Checking in nss/nss.def;         new revision: 1.156; previous revision: 1.155
Checking in pk11wrap/pk11mech.c; new revision: 1.4; previous revision: 1.3
Checking in pk11wrap/pk11obj.c;  new revision: 1.11; previous revision: 1.10
Checking in pk11wrap/pk11pub.h;  new revision: 1.12; previous revision: 1.11
Checking in pk11wrap/secmodi.h;  new revision: 1.23; previous revision: 1.22

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Checked in the nss/lib/ssl changes.

Checking in ssl/config.mk;   new revision: 1.20; previous revision: 1.19
Checking in ssl/derive.c;    new revision: 1.2;  previous revision: 1.1
Checking in ssl/manifest.mn; new revision: 1.13; previous revision: 1.12
Checking in ssl/ssl.h;       new revision: 1.23; previous revision: 1.22
Checking in ssl/ssl3con.c;   new revision: 1.73; previous revision: 1.72
Checking in ssl/ssl3ecc.c;   new revision: 1.2;  previous revision: 1.1
Checking in ssl/ssl3gthr.c;  new revision: 1.7;  previous revision: 1.6
Checking in ssl/sslauth.c;   new revision: 1.15; previous revision: 1.14
Checking in ssl/sslcon.c;    new revision: 1.28; previous revision: 1.27
Checking in ssl/ssldef.c;    new revision: 1.10; previous revision: 1.9
Checking in ssl/sslgathr.c;  new revision: 1.8;  previous revision: 1.7
Checking in ssl/sslimpl.h;   new revision: 1.40; previous revision: 1.39
Checking in ssl/sslinfo.c;   new revision: 1.14; previous revision: 1.13
Checking in ssl/sslnonce.c;  new revision: 1.17; previous revision: 1.16
Checking in ssl/sslsecur.c;  new revision: 1.32; previous revision: 1.31
Checking in ssl/sslsnce.c;   new revision: 1.35; previous revision: 1.34
Checking in ssl/sslsock.c;   new revision: 1.39; previous revision: 1.38

Saul, please check-in your patch next.  Thanks.

Comment 36

[Saul Edwards]

Attachment 195240 [details] [diff] checked in to trunk:

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.67; previous revision: 1.66
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.43; previous revision: 1.42
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.42; previous revision: 1.41
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61; previous revision: 1.60
done

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Fix regression caused by new noLocks feature

The new feature for enabling and disabling locks on a socket has a bug. 
When we disable locks on a socket that is using them, we forget to unlock two 
of the monitors.  When we later attempt to destroy those monitors, this bug 
causes an assertion failure in debug builds on all platforms except Solaris. :(

This patch fixes it.  

Julien, please review.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the review, Julien.
Checking in sslsock.c;  new revision: 1.40; previous revision: 1.39

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Attachment: plug leaks in HMAC code and SSL Bypass (method b)

In NSS 3.11, I added an new function HMAC_Init, which is just like HMAC_Create
except that HMAC_Init doesn't allocate the HMAC object, and HMAC_Create does.
Function HMAC_Destroy assumes that the HMAC context was created by 
HMAC_Create and always attempts to free it.  

LibSSL calls HMAC_Init repeatedly on the same pre-allocated context, but 
never called HMAC_Destroy.  If it had called HMAC_Destroy, it would have 
freed an address that was not previously returned by malloc.  A consequence
of calling HMAC_Init repeatedly without intervening calls to HMAC_Destroy 
is that each HMC_Init call after the first on the SSL socket leaks an 
MD5 or SHA1 context that was allocated by the previous call to HMAC_Init.

There appear to be two ways to fix this. 
a) change HMAC_Destroy to take an extra argument, "freeit", as many other
freebl destroy functions do.  We can do this now, because we have not yet
made an NSS release that exposes the HMAC_Destroy function.

b) have the HMAC context remember whether it was created by HMAC_Init or 
HMAC_Create, and have HMAC_Destroy free the context if it was allocated by
HMAC_Create.  

Both of these require that libSSL add a missing call to HMAC_Destroy, but
the arguments passed would differ.  

In this patch, I have implemented method b.  It passes all.sh and 
purify on selfserv with and without the bypass enabled. Please review it.  
If you object to method B and prefer method A, please advise ASAP.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Attachment: plug HMAC leaks (method a)

This code passes all.sh, but I haven't yet checked to see if it has any leaks, 

so I am not yet requesting review.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195846 [details] [diff] [review]
plug HMAC leaks (method a)

I have tested this patch with selfserv in the non-bypass mode, and found no new
leaks.

This patch makes the HMAC_Destroy signature match the signatures of some of the
other Destroy functions.  

But I have less confidence in this patch than in the other one, because this
patch relies on all the callers to correctly keep track of how the HMAC context
was allocated, but the other patch keeps track of it in the context itself,
where only the context initialization code needs to get it right.

Comment 42

[Wan-Teh Chang]

Comment on attachment 195839 [details] [diff] [review]
plug leaks in HMAC code and SSL Bypass (method b)

>+    if (cx->wasCreated)
>+	PORT_ZFree(cx, sizeof(HMACContext));

This should be:

      PORT_Memset(cx, 0, sizeof(HMACContext));
      if (cx->wasCreated)
	PORT_Free(cx);

You should also set cx->wasCreated to PR_TRUE in HMAC_Clone.
Note that HMAC_Clone is dead code, so you can also just remove
that function.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #42)
> (From update of attachment 195839 [details] [diff] [review] [edit])
> >+    if (cx->wasCreated)
> >+	PORT_ZFree(cx, sizeof(HMACContext));
> 
> This should be:
> 
>       PORT_Memset(cx, 0, sizeof(HMACContext));
>       if (cx->wasCreated)
> 	PORT_Free(cx);

I deliberately chose NOT to do it that way for performance.  
Note that Created is used in the softoken (FIPS) case, and not in the bypass 
case.  So I believe it suffices for FIPS to clear the HMAC context when it 
is Created and not when Inited.  
Please reconsider your review evaluation in that light.

> You should also set cx->wasCreated to PR_TRUE in HMAC_Clone.

Agreed.  

> Note that HMAC_Clone is dead code, so you can also just remove
> that function.

Isn't it cited in ldvector.c ?  (I'll double check).

Comment 44

[Wan-Teh Chang]

Comment on attachment 195846 [details] [diff] [review]
plug HMAC leaks (method a)

I like this patch (method a) better because the 'freeit'
parameter is very common in NSS.  (It's also used by
SECITEM_FreeItem.)

>Index: softoken/pkcs11c.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v
>retrieving revision 1.67
>diff -u -5 -r1.67 pkcs11c.c
>--- softoken/pkcs11c.c	13 Aug 2005 00:09:26 -0000	1.67
>+++ softoken/pkcs11c.c	13 Sep 2005 07:53:22 -0000
>@@ -122,11 +122,11 @@
> }
> 
> static void
> sftk_HMAC_Destroy(HMACContext *context, PRBool freeit)
> {
>-    HMAC_Destroy(context);
>+    HMAC_Destroy(context, freeit);
> }
> 
> static void
> sftk_Space(void *data, PRBool freeit)
> {

A better way to implement this change is to remove the
sftk_HMAC_Destroy function and replace
  context->hashdestroy = (SFTKDestroy) sftk_HMAC_Destroy;
with
  context->hashdestroy = (SFTKDestroy) HMAC_Destroy;

(The purpose of the sftk_HMAC_Destroy function is to
handle the "impedance mismatch" of function prototypes.)

The above code is another reason that method a is better.

I am troubled by the original code.  The original code
ignores the 'freeit' parameter.  This means either the
softoken may free memory that it shouldn't free, or the
softoken never calls the context->hashdestroy callback with
freeit=PR_FALSE.  My LXR queries showed that the latter is
true.

Comment 45

[Wan-Teh Chang]

Nelson, I trust your judgement of what is secure and what
is not secure.  The reason I suggested

      PORT_Memset(cx, 0, sizeof(HMACContext));
      if (cx->wasCreated)
	PORT_Free(cx);

was not for FIPS or security, but simply for the new code
to match the original code as closely as possible.

I do want to stress that the need to zeroize the
buffers that contain cryptographic keys or other sensitive
data (e.g., passwords) is not driven by FIPS but driven
by security.  Even though SSL PKCS#11 bypass code is not
FIPS compliant, it should still be secure.  I don't know
if the 'ipad' and 'opad' buffers in HMACContext will contain
any sensitive data at the end of an HMAC operation off the
top of my head.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Attachment: plug leaks - plan c

This patch is a hybrid of the previous patches A and B.
It adds the freeit argument to HMAC_Destroy, as in plan A.
It adds a "wasAllocated" member to the HMAC context, as in plan B.
HMAC_Destroy uses that member as a sanity check, to be sure it is 
not being asked to free unallocated memory.  

Julien, please look at Wan-Teh's comments about patches A and B,
and review this to see if it addresses all his concerns.  Thanks.

Comment 47

[Julien Pierre]

Comment on attachment 195998 [details] [diff] [review]
plug leaks - plan c

The patch looks fine.

Before RTM, it would be nice to surround the declaration of wasAllocated with
#ifdef DEBUG, since the field is not used in optimized builds at all and is
just a (very small) waste of memory. This would require using a macro for the 3
assignments of that field, or just surrounding them all with #ifdef DEBUG .

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

plug leaks - plan c,  checked in.
Checking in freebl/alghmac.c;   new revision: 1.3;  previous revision: 1.2
Checking in freebl/alghmac.h;   new revision: 1.4;  previous revision: 1.3
Checking in freebl/loader.c;    new revision: 1.23; previous revision: 1.22
Checking in freebl/loader.h;    new revision: 1.17; previous revision: 1.16
Checking in freebl/tlsprfalg.c; new revision: 1.4;  previous revision: 1.3
Checking in softoken/lowpbe.c;  new revision: 1.14; previous revision: 1.13
Checking in softoken/pkcs11c.c; new revision: 1.68; previous revision: 1.67
Checking in ssl/ssl3con.c;      new revision: 1.74; previous revision: 1.73

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Attachment: eliminate SSLNOLOCKS, add SSLFORCELOCKS, make FDX option mutually exclusive with NOLOCKS

This patch makes these changes:
1) eliminates the SSLNOLOCKS environment variable.  It is no longer possible
to make an application stop using SSL socket locks through an environment 
variable.

2) adds SSLFORCELOCKS environment variable.  Allos a user to force an
application to use SSL socket locks, even if the application tells libSSL not
to use them.

3. Makes two SSL socket options mutually exclusive.  An SSL socket can not 
be marked for FDX (simulteaneous use by separate reader and writer threads) 
while its locks are disabled, and its locks cannot be disabled if it is 
marked for FDX use. 

4. Use strings to record whether locks have been disabled, or forced. 
THis hsould make it possible to determine that without using a debugger.
The command 
  strings core | grep 'Locks are' 
should answer the question.

Comment 50

[Saul Edwards]

(In reply to comment #49)

I tested changes 1, 2, and 4 of Nelson's patch 196974 using selfserv and
strsclnt.  Trying all four combinations of settings, (global option set
true/false, environment variable SSLFORCELOCKS set/unset) I verified that both
performance and the string found in a corefile of the process matched expected
results.  

Note: if locks are left enabled by the application but SSLFORCELOCKS is also
set, the string in the process space reads "Locks are FORCED."

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Saul, please do one more test.  run selfserv with the SSLFORCELOCKS
variable, and kill selfserv in such a way that it makes a core file (e.g. 
kill -ABRT pid) and then run the strings comment (in comment 49) on the core 
file to see if the string is found.

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #51)
D'oh!  I see Saul wrote that he already did that test. Thanks Saul.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196974 [details] [diff] [review]
eliminate SSLNOLOCKS, add SSLFORCELOCKS, make FDX option mutually exclusive with NOLOCKS

Wan-Teh, please review.

Comment 54

[Wan-Teh Chang]

Comment on attachment 196974 [details] [diff] [review]
eliminate SSLNOLOCKS, add SSLFORCELOCKS, make FDX option mutually exclusive with NOLOCKS

Please ask someone else to review this patch. I need
to spend more time on FIPS validation.	Sorry.

Comment 55

[Robert Relyea]

Comment on attachment 196974 [details] [diff] [review]
eliminate SSLNOLOCKS, add SSLFORCELOCKS, make FDX option mutually exclusive with NOLOCKS

r=relyea

little nits:
comment on lockStatus declaration to warn that it  can be changed and must be
in data. (so we don't get it changed to const *lockStatus = someday;).

locksEverDisabled is still never really used. I presume it's a diagnostic tool.

lockstatus + 10 is a bit magic. I always prefer to have these things self
calculating... something like:
#define LOCK_STRING_PREFIX "Locks are "
#define LOCK_STRING_SUFFIX "ENABLED.   " /* must be long enough to hold the
string "DISABLED." */
#define LOCK_STRING_OFFSET sizeof(LOCK_STRING_PREFIX) -1
char lockStatus[] = LOCK_STRING_PREFIX LOCK_STRING_SUFFIX;

The #define strings don't need to be as long.

A less Macro Heavy way that is still acceptable is just under lockStatus
declaration:
#define LOCK_STATUS_OFFSET 10 /* start of ENABLED */
So someone looking at this codes knows they should modify the offset if the
modify the string.

lockStatus never gets set back to enabled, even if the locks are reenabled. I'm
presuming that this is intentional.

I'm presuming lockStatus is meant for both debugging and doing a strings | grep
on core files. If at least the latter is true the code is fine (my first
instinct is to put it in #ifdef DEBUG, but on further reflection, I think it's
desirable to have it in OPT builds as well.

The "FORCED. " string doesn't necessarily need the spaces, since you are using
strcpy. strcpy will write NULL byte at the end of the string, so there is no
need to clear  out the extra characters behind the string.

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196974 [details] [diff] [review]
eliminate SSLNOLOCKS, add SSLFORCELOCKS, make FDX option mutually exclusive with NOLOCKS

I made Bob's suggested change of replacing "10" with a defined symbol.

Checking in sslsock.c;	new revision: 1.43; previous revision: 1.42

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Attachment: reduce unnecessary allocation and zeroing of certain buffers

Saul, this patch responds to your observation about time spent in PORT_Zfree
on the trunk that was not spent on the branch.	Please see if this addresses
that issue adequately.	I suppose we need to redo leak testing after this.

Comment 58

[Saul Edwards]

Comment on attachment 197130 [details] [diff] [review]
reduce unnecessary allocation and zeroing of certain buffers

This patch gives us the expected 3% on specweb AMD64 and passes QA.

Comment 59

[Julien Pierre]

Nelson, is there anything left to do in this bug ? If not, please mark it fixed. Thanks.

Comment 60

[Julien Pierre]

Per our meeting today, this was fixed by the bypass.