Open
Bug 296487
Opened 19 years ago
Updated 2 years ago
bltest -H -a produces garbage and -b crashes
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
ASSIGNED
People
(Reporter: nelson, Unassigned)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•19 years ago
|
||
Targetting at 3.11, marking P1 because I think this should be a release stopper.
Priority: -- → P1
Summary: bltest -H -a crashes or produces garbage → bltest -H -a produces garbage and -b crashes
Target Milestone: --- → 3.11
Reporter | ||
Comment 2•19 years ago
|
||
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.
Reporter | ||
Updated•19 years ago
|
Assignee: wtchang → nelson
Status: NEW → ASSIGNED
Attachment #185243 -
Flags: review?(julien.pierre.bugs)
Reporter | ||
Comment 3•19 years ago
|
||
Comment on attachment 185243 [details] [diff] [review] patch part1 I am about to attach a bigger patch that fixes more bugs.
Attachment #185243 -
Attachment is obsolete: true
Attachment #185243 -
Flags: review?(julien.pierre.bugs)
Reporter | ||
Comment 4•19 years ago
|
||
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.
Attachment #185310 -
Flags: review?(julien.pierre.bugs)
Reporter | ||
Comment 5•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #185310 -
Flags: review?(julien.pierre.bugs) → review-
Reporter | ||
Comment 7•19 years ago
|
||
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.
Reporter | ||
Updated•19 years ago
|
Attachment #185310 -
Attachment is obsolete: true
Reporter | ||
Comment 8•19 years ago
|
||
Discovered that the bltest selftest command (-T) has *NEVER* correctly done a single test on windows, ever. This patch fixes that, too.
Attachment #185317 -
Attachment is obsolete: true
Reporter | ||
Comment 9•19 years ago
|
||
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.
Attachment #185318 -
Flags: review?(julien.pierre.bugs)
Comment 10•19 years ago
|
||
Comment on attachment 185318 [details] [diff] [review] patch v4 Remove review request because this patch no longer applies to the tip.
Attachment #185318 -
Flags: review?(julien.pierre.bugs)
Reporter | ||
Comment 11•19 years ago
|
||
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.
Attachment #185318 -
Attachment is obsolete: true
Reporter | ||
Comment 12•19 years ago
|
||
(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.
Reporter | ||
Comment 13•19 years ago
|
||
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.
Attachment #187468 -
Flags: review?(wtchang)
Comment 14•19 years ago
|
||
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?
Reporter | ||
Comment 15•19 years ago
|
||
Comment on attachment 187468 [details] [diff] [review] Deal with multi-threaded issues also ok
Attachment #187468 -
Flags: review?(wtchang)
Reporter | ||
Updated•19 years ago
|
QA Contact: jason.m.reid → tools
Reporter | ||
Comment 17•17 years ago
|
||
remove target milestone, since the target was missed.
Target Milestone: 3.11 → ---
Reporter | ||
Updated•15 years ago
|
Assignee: nelson → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•