Closed
Bug 337027
Opened 18 years ago
Closed 18 years ago
Coverity 506, dead code in mozilla/security/nss/lib/ssl/sslmutex.c
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.2
People
(Reporter: jonsmirl, Assigned: nelson)
Details
(Keywords: coverity)
Attachments
(2 files)
769 bytes,
patch
|
nelson
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
998 bytes,
patch
|
wtc
:
review-
|
Details | Diff | Splinter Review |
sslMutex_Init() if (!shared) return single_process_sslMutex_Init(pMutex); The !shared case is handled by a different routine. Later code in sslMutex_Init handling the !shared case is not accessible. @@ -143,17 +143,6 @@ if (err) { return err; NONBLOCKING- /* close-on-exec is false by default */ - if (!shared) { - err = fcntl(pMutex->u.pipeStr.mPipes[0], F_SETFD, FD_CLOEXEC); - if (err) - goto loser; - - err = fcntl(pMutex->u.pipeStr.mPipes[1], F_SETFD, FD_CLOEXEC); - if (err) - goto loser; - } - #if NONBLOCKING_POSTS err = setNonBlocking(pMutex->u.pipeStr.mPipes[1], 1); if (err)
Assignee | ||
Updated•18 years ago
|
Hardware: PC → All
Target Milestone: --- → 3.11.2
Version: unspecified → 3.11
Assignee | ||
Updated•18 years ago
|
Severity: normal → trivial
Priority: -- → P3
Attachment #221214 -
Flags: review?(wtchang)
Assignee | ||
Comment 2•18 years ago
|
||
Comment on attachment 221214 [details] [diff] [review] remove dead code This code is indeed dead. But it should be somewhere where it is not dead. It performs a function that is necessary under some circumstances, and the fact that it is not now ever being used is a bug. So the real "fix" is to figure out where (under what circumstances) this code should be being executed and fix it to be executed under those circumstances.
Attachment #221214 -
Flags: review?(wtchang) → review-
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → nelson
Assignee | ||
Comment 3•18 years ago
|
||
Question for Wan-Teh, Alexei, Julien, or anyone who can solve this riddle. In reviewing this code I wrote 5 years ago, It's no longer clear why I enclosed the code that that sets the close-on-exec flags on the pipes inside of if (!shared) { ... } !shared signifies the single-process server case, wherein we don't have multiple processes sharing the server session cache in shared memory, and therefore don't need pipes to use as semaphores, but we may have multiple threads running in the same process. Does that make sense to you? If anything, it seems like that should have been enclosed in if (shared) { } In any case, I'm thinking now that teh solution to this "dead code" problem is to remove the if (!shared) { and matching "}" from around that code that sets close on exec. Does that seem really wrong to anyone?
Assignee | ||
Comment 4•18 years ago
|
||
Does this make more sense?
Attachment #222465 -
Flags: review?(alexei.volkov.bugs)
Comment 5•18 years ago
|
||
Comment on attachment 222465 [details] [diff] [review] Revive dead code Nelson, did you test this patch? If the pipe is not inheritable, the child processes won't be able to use it.
Attachment #222465 -
Flags: review?(alexei.volkov.bugs) → review-
Comment 6•18 years ago
|
||
Comment on attachment 221214 [details] [diff] [review] remove dead code I think it's fine to just remove the dead code. If you really want to set close-on-exec, we can do that in the child processes. This will ensure that CGI programs executed by the child processes won't access to the pipes. But CGI programs executed by the parent may still have access to the pipes. You can start with SSL_InheritMPServerSIDCacheInstance to find the right place to add the fcntl calls to set close-on-exec.
Attachment #221214 -
Flags: review+
Assignee | ||
Comment 7•18 years ago
|
||
Wan-Teh, I don't follow you. Why would this patch prevent child server processes (the result of fork, but not exec) from using the pipes? This patch re-enabled code that sets close-on-exec (not close-on-fork). It's intended to allow children (which have forked but not execed) to continue to use the pipes, but to disallow other executables (which would come from exec) to use them or inherit them. close-on-exec benefits exec'ed code about which NSS knows nothing, such as code run by the application (through fork and exec) wihtout NSS's knowledge.
Comment 8•18 years ago
|
||
Comment on attachment 222465 [details] [diff] [review] Revive dead code Nelson, I forgot that close-on-exec applies to exec, not fork. Sorry to have confused you. But selfserv calls PR_CreateProcess, not fork, to create the child processes. PR_CreateProcess does a fork+exec sequence. So the pipe still won't be accessible in the selfserv child processes if you set close-on-exec in sslMutex_Init, right?
Assignee | ||
Comment 9•18 years ago
|
||
Thanks, Wan-Teh. for some reason, I was thinking we only used PR_CreateProcess on windows. We don't provide any way for applications to find the FD numbers of our pipes, IIRC. So there's no way for applications to avoid having their children inherit these live pipes. Seems like we ought to solve that. I've recently heard about some callback that a library can register that will get called in the parent before any fork, and in the child after a fork. Maybe the answer is to use that method so that the children of the NSS parent process set close-on-exec. But I don't know what platforms support it. Also, I wonder if perhaps some of the platforms that didn't support semaphores in shared memory 5 years ago (such as Linux) now do support it. Maybe we ought to revisit that big #if statement at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslmutex.c&rev=1.19#39
Comment 10•18 years ago
|
||
Nelson, the function you referred to is pthread_atfork.
Assignee | ||
Comment 11•18 years ago
|
||
Comment on attachment 221214 [details] [diff] [review] remove dead code i've decided to r+ this patch. I've filed bug 339466 about the issues related to the pipes and pthread_atfork.
Attachment #221214 -
Flags: review- → review+
Assignee | ||
Comment 12•18 years ago
|
||
Correction: I filed bug 339468 about the pipes, and bug 339466 about pthread_atfork .
Assignee | ||
Comment 13•18 years ago
|
||
Checking in sslmutex.c; new revision: 1.20; previous revision: 1.19 Checking in sslmutex.c; new revision: 1.19.28.1; previous revision: 1.19
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•