337027 - Coverity 506, dead code in mozilla/security/nss/lib/ssl/sslmutex.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Jon Smirl]

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)

Comment 1

[Jon Smirl]

Attachment: remove dead code

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Revive dead code

Does this make more sense?

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Wan-Teh Chang]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

Nelson, the function you referred to is pthread_atfork.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Correction:
I filed bug 339468 about the pipes, and bug 339466 about pthread_atfork .

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

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