286505 - certutil has infinite loop in interactive mode for cert extensions

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

The first checkin for bug 285208 had the effect of causing certutil to 
go into an infinite loop while in "interactive mode", gathering responses
to its questions about cert extensions.  In nightly QA builds, it output
gigabytes of prompts, eventually filling the disk.

The patch caused certutil to interactively ask about every cert extension 
twice, once while building the cert reuqest, and again while building the
cert  Consequently, the amount of input being provided to stdin by all.sh 
was no longer enough to answer all the questions.  I hypothesize that 
certutil encountered EOF on stdin, but failed to detect EOF, and kept
asking one or more questions over and over.

The point is that EOF on stdin during interactive mode should cause the 
process to STOP, not retry forever.  Because of the effect this had on
build and test machines, I consider this P1.

The infinite loop is no longer occuring because certutil is no longer
asking for every extension twice.  But we don't want certutil to ever
go into such an infinite loop again, merely because of insufficient 
input on stdin (or whatever the cause).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

marking P1 for 3.10

Comment 2

[Neil Williams]

There are 3 places in certutil's extension processing code where bad things can
happen when EOF happens on standard input. Options -3, -4, call GetGeneralName()
which keeps adding null names to its list until malloc fails. Options -5 and -6
each have a while (1) loop that doesn't check return codes from scanf() and gets().

I will have a patch this PM.

Comment 3

[Neil Williams]

Attachment: Changes to keep null standard input from causing infinite loop.

These changes keep certutil from looping forever when it sees EOF standard
input. There are several places where the return codes from gets() and scanf()
are ignored. I claim that the ones fixed by this patch are sufficient to keep
certutil out of trouble.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 177814 [details] [diff] [review]
Changes to keep null standard input from causing infinite loop.

One question:

> 	puts ("\nEnter data:");
> 	fflush (stdout);
>-	gets (buffer);
>+	if (gets (buffer) == NULL) {
>+            break;

Should this set rv to SECFailure and set an error code,
like the preceeding part of the patch did?  

>+        }

Please do this test (if you haven't already).  In a workarea, back out
your previous patch to certutil (which will recreate the problem that 
led to the crash) and apply this patch, then run all.sh (or just cert.sh) 
and confirm that that problem does not recurr.

Comment 5

[Neil Williams]

Attachment: Fix to prevent inifinite loop on EOF in std input

Several tests were made to confirm that what this patch fixes was indeed the
cause of the behaviour reported by QA. Note that the failure mode was different
on SPARC than on x86 Solaris with the SPARC version looping forever. I'll look
into this next.

Comment 6

[Neil Williams]

Comment on attachment 177814 [details] [diff] [review]
Changes to keep null standard input from causing infinite loop.

The first if (gets()... change needed to set error codes.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 177941 [details] [diff] [review]
Fix to prevent inifinite loop on EOF in std input

r=nelson
Please don't mark this bug resolved until we know why the results were so
different on S10/AMD64 than  on S9/sparc.

Comment 8

[Neil Williams]

Checking in nss/cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.92; previous revision: 1.91
done

Comment 9

[Neil Williams]

Looking at the differences in behavior on x86 and SPARC it is clear that an
uninitialized buffer is to blame. The offending code was like this at the start
of AddNscpCertType():
    char buffer[5];
...
    gets (buffer);
    value = atoi (buffer);
    if (value < 0 || value > 7)
	break;
...
When gets sees EOF buffer is left as it was on entry to the function. Since
atoi() of any alpha char is 0 this happens fairly frequently. Based on this the
bug is being marked resolved/fixed.