903565 - Crash in ssl_NewSocket in failure case

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Ryan Sleevi]

Bug 681065 had a merge issue when landing, such that the socket protocol variant was assigned after all the other fields were initialized. However, the assignment of this field was included in the loser: case, which means it may cause a NULL memory dereference (and thus, a crash) if initializing the socket failed.

This was reported in Chromium as http://crbug.com/270735, with the sample stacktrace reproduced below:

The problem is that |ss| is used after it is reset to NULL:

	if (makeLocks) {
	    status = ssl_MakeLocks(ss);
	    if (status != SECSuccess)
		goto loser;                          // Will crash below
	}
	status = ssl_CreateSecurityInfo(ss);
	if (status != SECSuccess) 
	    goto loser;                              // Will crash below
	status = ssl_InitGather(&ss->gs);
	if (status != SECSuccess) {
loser:                                               // Will crash below
	    ssl_DestroySocketContents(ss);
	    ssl_DestroyLocks(ss);
	    PORT_Free(ss);
	    ss = NULL;                               // <-- clearing |ss|
	}
	ss->protocolVariant = protocolVariant;       // <-- CRASH
    }
    return ss;
}

0x62b26210	 [remoting_core.dll]	 - sslsock.c:3124]	ssl_NewSocket
0x62b26d12	 [remoting_core.dll]	 - sslsock.c:1415]	ssl_ImportFD
0x62b26d98	 [remoting_core.dll]	 - sslsock.c:1447]	SSL_ImportFD

Comment 1

[Ryan Sleevi]

Attachment: Fix for protocolVariant initialization

Comment 2

[Wan-Teh Chang]

Comment on attachment 788315 [details] [diff] [review]
Fix for protocolVariant initialization

r=wtc. Sorry about the bug!

Comment 3

[Ryan Sleevi]

Committed as https://hg.mozilla.org/projects/nss/rev/de33c29a6bde

Fixed in 3.15.2