Closed
Bug 1265127
Opened 8 years ago
Closed 7 years ago
Race condition in initialization code of ssl_PushIOLayer
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.35
People
(Reporter: philipp.unterbrunner, Assigned: jallmann)
Details
(Keywords: good-first-bug)
Attachments
(1 file)
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20160407164938 Steps to reproduce: I am working on an application that concurrently opens HTTPS connections through libcurl 7.42.1 on NSS 3.18. I checked the source code, and the bug persists at least until 3.23. I noticed occasional crashes during the first few HTTP connections opened by the application. The crashes happened when libcurl attempted to call function pointers that were supposed to have been initialized by ssl_SetupIOMethods. I eventually produced a multi-threaded core dump that showed that the crashing thread had proceeded past the SSL initialization code while another thread was still inside ssl_InitIOLayer, a function that is only called by ssl_PushIOLayer, where the function call is protected by double-checked locking and PR_CallOnce. I believe that the issue is the plain Boolean variable called ssl_inited that is used for double-checked locking. The compiler is free to reorder reads and writes to plain Boolean variables, and, alas, it makes use of that freedom in my binary. Here is proof: (gdb) l 2729 2730 static PRStatus 2731 ssl_InitIOLayer(void) 2732 { 2733 ssl_layer_id = PR_GetUniqueIdentity("SSL"); 2734 ssl_SetupIOMethods(); 2735 ssl_inited = PR_TRUE; 2736 return PR_SUCCESS; 2737 } 2738 (gdb) p ssl_inited $2 = 1 (gdb) frame #5 ssl_InitIOLayer () at sslsock.c:2734 (gdb) disassemble Dump of assembler code for function ssl_InitIOLayer: 0x000000346f4287e0 <+0>: lea 0xae39(%rip),%rdi # 0x346f433620 0x000000346f4287e7 <+7>: sub $0x8,%rsp 0x000000346f4287eb <+11>: callq 0x346f40a748 <PR_GetUniqueIdentity@plt> 0x000000346f4287f0 <+16>: mov %eax,0x21572a(%rip) # 0x346f63df20 <ssl_layer_id> 0x000000346f4287f6 <+22>: callq 0x346f40a208 <PR_GetDefaultIOMethods@plt> 0x000000346f4287fb <+27>: mov %rax,%rsi 0x000000346f4287fe <+30>: lea 0x45b(%rip),%rax # 0x346f428c60 <ssl_Close> 0x000000346f428805 <+37>: lea 0x215734(%rip),%rdi # 0x346f63df40 <combined_methods> 0x000000346f42880c <+44>: mov $0x24,%ecx 0x000000346f428811 <+49>: movl $0x1,0x215701(%rip) # 0x346f63df1c <ssl_inited> => 0x000000346f42881b <+59>: rep movsq %ds:(%rsi),%es:(%rdi) The assembly shows that ssl_inited is set to 1 before the call to ssl_SetupIOMethods. This causes double-checked locking to fail and allows other threads to proceed while ssl_InitIOLayer is still running. #7 0x000000346f42c6cb in ssl_PushIOLayer (ns=0x7fd48c0dd560, stack=0x7fd48c0dd500, id=-2) at sslsock.c:2746 2746 status = PR_CallOnce(&initIoLayerOnce, &ssl_InitIOLayer); (gdb) l 2741 { 2742 PRFileDesc *layer = NULL; 2743 PRStatus status; 2744 2745 if (!ssl_inited) { 2746 status = PR_CallOnce(&initIoLayerOnce, &ssl_InitIOLayer); 2747 if (status != PR_SUCCESS) 2748 goto loser; 2749 } The fix is to either remove ssl_inited and always call PR_CallOnce (at the cost of some performance), or make sure the compiler does not reorder accesses to ssl_inited. The best way to do the latter is platform dependant. I assume the Mozilla libraries have some utility code for this purpose (atomic Booleans and such). Actual results: Crash in libcurl Expected results: No crash ;)
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Updated•7 years ago
|
Updated•7 years ago
|
Priority: -- → P3
Comment 1•7 years ago
|
||
Comment on attachment 8932454 [details] Bug 1265127, fixed race condition in ssl_PushIOLayer, r=franziskus Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision. https://phabricator.services.mozilla.com/D293#7151
Attachment #8932454 -
Flags: review+
Comment 2•7 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/c71d2fa1a53c6039f40878bf66be90a6bbc927c9
Assignee: nobody → jallmann
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.35
You need to log in
before you can comment on or make changes to this bug.
Description
•