296487 - bltest -H -a produces garbage and -b crashes

Status: ASSIGNED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is related to bug 292763, but doesn't necessarily block it, IMO

I'm doing some performance work on MD5, and using bltest to measure the 
performance after each modification.  I used the command:
   bltest -H -m md5 -a -i inputfile -p 50000 -b 20000

Even though the algorithm was correct in every test, and the input file to 
the test was the same every time, I found that I was getting 
DIFFERENT RESULTS EACH TIME.  

When I tried it with a different input file, it crashed.  

So, I built a debug build and looked into it and found these things:

1) the -a command line option, which causes the output to be given in 
Base64 enocding, also causes the program to expect the INPUT to be base64
encoded.  The program attempts to decode the input, and if the input is
not valid base64-encoded data, the program silently ignores this failure.
Consequently, the program ends up hashing an incompletely initialized 
buffer, resulting in random hashes being computed.  BTW, this behavior
is apparently common to all the bltest commands, not unique to hashing.
BLtest needs to be able to specify the binary/ascii attribute for input
separately from the output.

2) in "binary mode" (when the -a option is not given) the program reads in
the input file and then removes any trailing \r or \n characters from the
file!  Not unique to hashing.

3) The program does not detect when the size of the input file named in 
the -i command line option is smaller than the buffer size specified with
the -b command line option.  This causes a crash (heap corruption) every time.
Again, this is not unique to hashing.

4) There is a test function for each of the hash algorithms.  Each function
invokes its respective hash algorithm's Init, Update, and End/Finalize 
methods.  All those end methods take an argument that is the maximum size
of the output buffer, into which the computed has is to be placed.  
All these functions pass the same value, which is the size of an MD5 hash,
to their respective hash algorithms' End method.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Targetting at 3.11, marking P1 because I think this should be a release stopper.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part1

I have fixed some of the problems, including another one I found. 
This patch addresses some but not all of the issues described above.  
It is not the full and complete fix for this bug, but it is a big start, IMO.

- It does NOT address the issue of the -a option affecting both input and
output

- When the input is expected to be base64, and fails to decode, the patched
code detects the failure and terminates gracefully with an error message.

- It detects the combination of -i filename and -b buffersize (which previously

attempted to overwrite the input file with buffersize bytes of random data!),
and now reports that this combination is unsupported.  This is not a real fix,
but it at least stops the insane behavior and associated crashes.

- It corrects the size values passed to all the hash algorithms' End functions.


all.sh passes on my system with this patch in place.  

Here is one more issue I discovered with bltest.  On windows, when reading or
writing binary data on stdin or stdout, bltest does not do the necessary
setmode calls that stop windows from turning \r\n into \n on input or 
changing \n to \r\n on output.	I'll deal with that in my next patch.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 185243 [details] [diff] [review]
patch part1

I am about to attach a bigger patch that fixes more bugs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

This patch fixes all the issues discussed above except 1. 
The input and output files are always in the same mode, that is, both are 
assumed to be in the same one of these 4 formats:
  - raw binary
  - base64 encoded ASCII  (-a)
  - hex ASCII		  (-h)
  - hex ASCII, with bytes separated by spaces (-l)
Fixing that problem, so that the input and output can have separate 
formats, requires an change or extension to the program's option syntax.
So, I am foregoing that at this time.

Julien, please review.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

One more issue discovered about bltest.  

bltest has a -N command, which is supposed to output a value derived
from a randomly generated input, passed through a crypto algorithm.  
The implementation of that feature depended on the behavior described
above, which attempted to overwrite the INPUT file with random data.  

I think the -N command was used to generate values that are now the 
data files checked in under cmd/bltest/tests.  These files contain
known good input/output pairs for all the crypto algorithms, and are 
used for regression testing of the algorithms themselves.  

The -N command does not work.  It probably crashes.  Fixing it seems
unimportant at the moment.

Comment 6

[Julien Pierre]

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

This patch introduces a regression. When doing "bltest -H -m sha1 -p 131072 -b
8192", I get a hang , which I didn't previously get. This is because bltest is
now reading from stdin, where it previously wasn't.

Also -b without a value crashes bltest, and should be fixed at the same time.

You reported that there were other issues relating to CR/LF, so I didn't review
all of the patch.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3

Julien found that bltest with the -b bufsize option but without the -i filename
option should generate random input, rather than read bufsize bytes from stdin.
 
So, I changed the code to work that way.

I also found that ALL of the self-test data files (files found in
nss/cmd/bltest/tests/... used with the -T command) are actually ASCII files
that end with LF on unix and with CRLF on windows.  None of these files have
the -kb flag set.  Some, but not all, of these files are base64 encoded. 
The program has hard-coded knowledge about which ones are base64, and which 
are not.  The ones that are not are treated as "binary" files, except that
it expects the trailing LF or CRLF to be removed first.  So, I changed the
program to remove trailing LF or CRLF from "binary" input files ONLY when
in selftest mode (-T).	

Finally, I found that the source code used variables named "mode" to 
sometimes mean "algorithm" and sometimes mean "file format".  So I changed
the mode variables that meant file format to be named ioFormat.  Now the 
word mode always denotes the selected crypto algorithm in the source code.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v4

Discovered that the bltest selftest command (-T) has *NEVER* correctly
done a single test on windows, ever.  This patch fixes that, too.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 185318 [details] [diff] [review]
patch v4

Now I have seen this work on both windows and Solaris. 
Julien, please review again.  Thanks.

Comment 10

[Julien Pierre]

Comment on attachment 185318 [details] [diff] [review]
patch v4

Remove review request because this patch no longer applies to the tip.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Deal with multi-threaded issues also

This patch is huge. 
It does all the things that the previous patch did, and much more.  
It passes cipher.sh and performance.sh.  It fixes output offsetting (the 
-2 option) for multiple repetitions and for multiple threads.
It fixes the time accounting for multiple threads.  It also does the input
and setup only once, no matter how many threads are being used.  Previously
bltest would read all the input files once for every thread.
It replaces the old Usage with a new one that is much shorter but documents
all the implemented options.
It makes the mode listing feature work according to the Usage.
It ONLY outputs files containing new keys, IVs, and input data when the -N 
option is given.  Those new files are always named tmp.key, tmp.iv and tmp.in.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)

bltest's -N command is vital to the performance.sh script. So I fixed it.

In NSS 3.10, whenever bltest needed an input that was not supplied (a key, 
an IV, an input buffer, PQG params, etc.) it would generate the needed input
and then write it to a file in $PWD, named tmp.key, tmp.in, or tmp.in.  
Any command that requires any of those inputs (e.g. -E, -D, -H etc.) will
generate them and put them into the "tmp.XXX" files.  
The -N command essentially tells bltest to do that generation of missing 
input and then stop there.  It is used to generate sample test cases.  
It is also used to measure key generation time.  
The performance.sh script uses the -N command heavily.

With this latest patch, the tmp.XXX files are ONLY generated when the -N
command is given.  Those files are no longer ever created as a side effect
of the other commands (-D -E -H, etc.)

I discovered one bug with the generation of random input.  This can cause the
RSA and DSA tests to fail.  If the random input is numerically greater than 
the modulus, the RSA/DSA code will return an error.  bltest really should 
reduce the input to be less than the key's modulus.  For now, that behavior
is just a "known bug", that has been present in bltest for years.

I have checked the above 107KB patch in on the Performance hacks branch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187468 [details] [diff] [review]
Deal with multi-threaded issues also

I was mistaken in thinking that I had previously marked this patch as wanting
review.  

This patch is MUCH larger than the previous one because it fixes numberous
issues that have recently arisen.

Comment 14

[Wan-Teh Chang]

Comment on attachment 187468 [details] [diff] [review]
Deal with multi-threaded issues also

This is a huge patch. Could you ask someone else to review it?

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187468 [details] [diff] [review]
Deal with multi-threaded issues also

ok

Comment 16

[Julien Pierre]

Per our meeting today, lowering to P2.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.