certutil has infinite loop in interactive mode for cert extensions

RESOLVED FIXED in 3.10

Status

NSS
Tools
P1
critical
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Neil Williams)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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).
(Reporter)

Comment 1

13 years ago
marking P1 for 3.10
Priority: -- → P1
Target Milestone: --- → 3.10
(Assignee)

Comment 2

13 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

13 years ago
Created attachment 177814 [details] [diff] [review]
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.
Attachment #177814 - Flags: review?(nelson)
(Reporter)

Comment 4

13 years ago
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.
(Assignee)

Comment 5

13 years ago
Created attachment 177941 [details] [diff] [review]
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.
Attachment #177941 - Flags: review?(nelson)
(Assignee)

Comment 6

13 years ago
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.
Attachment #177814 - Attachment is obsolete: true
Attachment #177814 - Flags: review?(nelson)
(Reporter)

Comment 7

13 years ago
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.
Attachment #177941 - Flags: review?(nelson) → review+
(Assignee)

Comment 8

13 years ago
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
(Assignee)

Comment 9

13 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.