225513 - NISCC SMIME tests require tool that decodes multiple messages

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Today, the cmsutil program decodes a single message and then quits. 
This is suboptimal for leak detection because per-message leaks only appear once. 

This also makes crash detection more difficult.  To test 400k test cases,
and determine if any of them caused a crash, it is necessary to keep a log
file and examine its output.

For the NISCC SMIME tests, ideally a single cmsutil process would be able to
read and decode a series of messages until either it finishes or it crashes.  

The series of files to be tested could either be 

a) determined algorithmically (going through numbered file names in sequence), or
b) read from a file of file names.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P2 for NSS 3.9.  We _could_ do the NISCC testing without this feature,
but IMO, we will get more meaningful results with this feature available.

Comment 2

[Julien Pierre]

What you want is some sort of batch mode.
As I recall, the cmsutil program structure is very oriented toward the
single-transaction model.

One of the reasons is that it is written to be integrated with the smime perl
script which does the MIME processing. cmsutil uses stdin/stdout pipes, or
filenames, to work with the perl script. Therefore, to truly make batch mode
work for cmsutil, the MIME code from the perl script should be rewritten in C
into cmsutil. This is something we have wanted to do for a while, but I think it
is a requirement for this project.

As far as the method to process multiple filenames, I think either wildcards
could be used for the input file names, or a filename containing a list of input
files as you suggest.

There would have to be a rule for the output file names though; I think
appending ".decrypted", ".encrypted", ".signed" suffixes to each input's
filename would work.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The NISCC "SMIME" test cases are all raw (binary) CMS (PKCS7) messages.  
They are not base64 encoded and they are not put into a MIME (S/MIME) format.
Some signatures are detached, others are not.  
So the NISCC tests do not necessitate writing the SMIME parser in c.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - see comments below

This patch depends on another patch that has not yet been checked in 
because of the review process.	

This patch implements a new "batch" option for cmsutil.  A new command line
option, -b modifies the behavior of the DECODE function (-D option).  
It causes the input file (either stdin or as specified by the -i option)
to be interpreted not as the file to decode, but rather as a list of 
files names to be decoded, one at a time.  
cmsutil reads each file, outputs a header line telling which file it is 
operating on, and then decodes that file.  It does not stop for decoding
errors, but it does stop if one of the named files cannot be opened.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 135865 [details] [diff] [review]
patch v1 - see comments below

Wan-Teh, please review.  
This patch depends on the patch v3 for Bugscape bug 53775.  Please review that
patch first.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

Fix an infinite loop at EOF in the batch file.
Like patch v1, this patch depends on another patch first.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 135865 [details] [diff] [review]
patch v1 - see comments below

This patch is obsolete.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 135894 [details] [diff] [review]
patch v2

Please review

Comment 9

[Robert Relyea]

Comment on attachment 135894 [details] [diff] [review]
patch v2

r=relyea
I see no bugs in the patch. A couple of comments:

1) p1_fgets carefully adds the EOL character while it's only caller carefully
discards that character. It seems a bit of a waste. I can see the logic of it
since presumably p1_fgets is matching the semantics of fgets. Perhaps calling
it p1_gets abd dropping the EOL char? (or just leaving it as is;).

2) p1_fgets should probably return NULL on EOF. doBatchDecode() appears to be
expect this semantic.

2) in doBatchDecode we set the value 'str' from the p1_fgets, but then never
use it. I'd suggest either removing it completely, or using it in the loop
instead of batchLine.

bob

Comment 10

[Wan-Teh Chang]

Comment on attachment 135894 [details] [diff] [review]
patch v2

r=wtc.	Some comments and suggested changes.

1. Julien implemented the batch mode for certutil recently.
Your batch mode is different from his as follows:
- '-b' vs. '-B' (cosmetic)
- Your batch file is a list of file names, one in each line.
  Julien's batch file contains arbitrary command-line options,
  each line for each invocation of certutil.

2. Add 'static' to pl_fgets and doBatchDecode.

3. Remove the commented-out test:
       *cp == '\r' ||
   in pl_fgets.

4. In doBatchDecode, you handle the failures of PR_Open and
SECU_FileToItem differently.  The former you exit the while
loop, the latter you continue.

5. It looks nicer to say
     if (batch) {A;} else {B;}
   than
     if (!batch) {B;} else {A;}

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Checked in 

/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.45; previous revision: 1.44

This checkin combines the patches for bugscape bug 53775 and bugzilla bug
225513, and the review comments from Bob Relyea and Wan-Teh Chang.

Bob and Wan-Teh, perhaps you should review this revision to be sure that 
the changes reflect your comments.

Marking fixed.