793033 - Remove the strange sslSocket copying in ssl_FreeSocket

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

In DEBUG build, ssl_FreeSocket makes a copy of the
sslSocket structure, overwrites the old sslSocket
structure with the 0x1f pattern, destroys the members
(using the new copy), and then frees the old sslSocket
structure. This strange sslSocket copying is in the
original rev. 1.1, so I can't find out from CVS history
what the motivation is. It is most likely a debugging
aid.

If any pointer member of the sslSocket structure points
to another member of the structure, that pointer member
in the new copy will be pointing to the wrong copy (the
old structure). This prevented Ekr from declaring a
PRCList as a member of the sslSocket structure. (PRCList
contains two pointers. If a PRCList is empty, the two
pointers point to the PRCList itself.)

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslimpl.h&rev=1.107&mark=819-823#819

Now, this strange sslSocket copying also messes up a
patch to extend AESContext with a pointer member that
points to allocated memory.  The event sequence is:

    AES_InitContext(&(some member of sslSocket), ...);
    ...
#ifdef DEBUG
    memset(ss, 0x1f, sizeof(*ss));
#endif
    ...
    AES_DestroyContext(&(some member of sslSocket), PR_FALSE);

In a debug build, that AESContext structure embedded in sslSocket
will have been overwritten with the 0x1f pattern by the time we
call AES_DestroyContext(..., PR_FALSE), so all the members of that
AESContext structure are messed up. This isn't a problem now
because AES_DestroyContext() is a no-op if 'freeit' is PR_FALSE.
If we change AES_DestroyContext() to inspect any member of the
AESContext strucutre when 'freeit' is PR_FALSE, things will go
wrong.

I think the blame should be assigned to the memset(ss, 0x1f, sizeof(*ss))
call before the AES_DestroyContext(..., PR_FALSE) call, rather than to
AES_DestroyContext(..., PR_FALSE). We should not mess up a structure
before passing it to a Destroy function.

I proposed that we remove the strange sslSocket copying from ssl_FreeSocket.

Comment 1

[Robert Relyea]

Comment on attachment 663199 [details] [diff] [review]
Proposed patch

r+ rrelyea

Comment 2

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.14).

Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.96; previous revision: 1.95
done