302585 - cmd/crlutil/crlgen.c using pointer count as byte count

Status: RESOLVED DUPLICATE of bug 298409

(NSS :: Tools, defect)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD) KHTML/3.4.1 (like Gecko)
Build Identifier: 

The MAX_EXT_DATA_LENGTH define in the file represents the maximum number of  
fields extension may have. Unfortunately, the call to PORT_Zalloc() on line  
1365 treats it as the number of total _bytes_ extensionsion fields may  
occupy...  
  
This breaks all hell loose on 64-bit machines (with 8-byte pointers you can 
only have one extension field before corrupting memory) and will 
quietly corrupt memory even on 32-bit ones.  
  
I'm attaching my patch for the file -- its beginning merely made the tool's  
diagnostics friendlier, but the last hunk is absolutely crucial to fixing this  
bug. 
 
The problem breaks tests (cert.sh) on 64-bit machines... 

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: Improve crlutil and fix a major problem in it

The last hunk in the patch is of vital importance.

Comment 2

[Julien Pierre]

Alexei,

Can you take a look at this bug report ?

Mikhail,

We have been running crlgen and cert.sh on 64-bit boxes successfully, including
Solaris sparcv9, amd64, hp-ux pa-risc, aix powerpc, linux amd64 . It's possible
that the allocator on all those platforms allocates more memory than needed as a
minimum and the problem was hidden, but seems unlikely. Which platform did you
run it on that showed a problem ?

Comment 3

[Mikhail T.]

I observed the problem on my home FreeBSD/amd64 system with non-default 
malloc.conf settings ("R<"): 
 
     R       Causes the realloc() and reallocf() functions to always reallo- 
             cate memory even if the initial allocation was sufficiently 
             large.  This can substantially aid in compacting memory. 
[...] 
     <       Reduce the size of the cache by a factor of two.  The default 
             cache size is 16 pages.  This option can be specified multiple 
             times. 
 
I then used Purify on Solaris (-xarch=v9), where the problem itself was not 
reproducible, but Purify reported a number of errors. Of these, the most 
obviously dangerous was this one. 
 
I can post complete Purify logs tomorrow, when I get back to the office. 
 
Yes, I think, your explanation for why this is usually hidden sounds 
reasonable. 
 
Why aren't you, guys, running your tests using Purify :-) ? 

Comment 4

[Mikhail T.]

Attachment: Improve crlutil and fix a major problem in it

Oopss.... I initially posted the wrong version of the patch -- without this
important hunk at the end :-(

Thanks to Joe Marcus Clarke for pointing it out.

Comment 5

[Mikhail T.]

By the way. The definitions of SECITEM_Hash and SECITEM_HashCompare in crlgen.c 
are redundant (also found in secitem.c) and, being non-static, prevent creation 
of statically linked crlutil. 
 
Apparently, the two functions can be simply removed from the file, together 
with the comment, which encourages such removal. 

Comment 6

[Wan-Teh Chang]

Thanks a lot for tracking down this bug and submitting
a patch.  This is a known bug and has been fixed on the
NSS_3_10_BRANCH.

*** This bug has been marked as a duplicate of 298409 ***

Comment 7

[Wan-Teh Chang]

The reason SECITEM_Hash and SECITEM_HashCompare were duplicated
in crlgen.c is that they are not exported from the nss3 shared
library.  Perhaps they should be exported.

Comment 8

[Mikhail T.]

Preferably SECITEM_Hash and SECITEM_HashCompare are "exported" on Windows. 
Because on all Unixes (where there is no need for explicit export) statically 
linking crlutil us currently not possible due to these two functions being 
multiply defined. 

Comment 9

[Julien Pierre]

Mikhail,

re: comment #3

Purify on Solaris has a lot of bugs, which IBM hasn't been willing to fix over
the years. The Purify problems mostly have to do with the handling of our Sparc
v8+ and Sparc v9 assembly code in freebl. Many NSS programs cannot run on
Solaris due to these Purify bugs. We would have to produce different builds of
NSS to use Purify on Solaris. This is why we don't use it.

Comment 10

[Mikhail T.]

Julien!  
 
=Many NSS programs cannot run on Solaris due to these Purify bugs. 
 
Of course, you can for two independent reasons: 
 
1) Purify can be instructed to suppress any warning -- if you know it is wrong 
about a particular function, you can add the suppression to ~/.purify. 
2) Even if some of the Purify's warnings are wrong (and you can't or don't want 
to suppress them for some reason), you can still "Purify" the test programs. 
Your goal is not neccessarily to have a clean Purify run, but, rather to check, 
that no NEW Purify warnings/errors are introduced by some development. This 
relative cleanness is quite achivable even if the absolute one is not. 
 
You can also run Purify on Linux -- I use Purify on both Linux and Solaris 
every day. It is the same version and even uses the same license-server. 
 
I wish they had a FreeBSD version, but they don't :-( 
 
Finally, from my experience of dealing with Rational (now IBM) support, I can 
tell, that they are quick to fix any bugs, that they can reproduce. Have you 
tried that? 

Comment 11

[Julien Pierre]

Mikhail,

I wasn't talking about warnings from purify, I was talking about code that can't
actually run properly when purified under solaris - functions that have
different results . Try selfserv for example to see what I mean. And no, I
haven't had the time to talk to IBM about it, unfortunately. Re: Linux, I don't
believe we have any licenses for Purify/Linux,  so that wouldn't be an option .
Some of us have been doing occasional Windows Purify runs, but not systematic,
and not on all tests .

Comment 12

[Robert Relyea]

Re comment #8. 'Exported' means exported from the DLL. If you statically link
anything, it is possible you might access functions where are 'public' in the .o
sense, but not public in the API sense.

Static linking is highly discouraged. We have some statically linked commands in
NSS, but they are statically linked for some reason other than 'I don't want to
copy the relevant shared libraries to run the program' (usually because the are
unit test programs which test non-public functions within NSS).

All that being said, I would say SECITEM_Hash and SECITEM_HashCompare are
classic functions we should supply from secutil and export publically.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Change subject to be descriptive of the problem