298409 - MSVC debug runtime library assertion failures in crlutil

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

I built NSS_3_10_RTM with NSPR_4_6_RTM using MSVC .NET 2003
(7.1) on Windows XP.  The build described in this bug report
is the OBJ.OBJD build; you need to set USE_DEBUG_RTL=1 when
building.

When I run all.sh, I get an assertion failure in crlutil from
the MSVC debug runtime library:

    Micrisoft Visual C++ Debug Library

    Debug Error!

    Program: ...1\spdpc1_win2k\mozilla\dist\win954.0_dbg.objd\bin\crlutil.exe

    DAMAGE: after Normal block (#2692) at 0x0055CE50

I can't debug crlutil.exe at this point, so I can't get
more information than this.

In my experience, such assertion failures report memory
errors such as freeing memory not allocated from the heap
or freeing memory that has already been freed.  So that
would be a good direction to investigate.

Comment 1

[Julien Pierre]

I was able to reproduce the problem with VC6.
Once again, MKS signal handling is getting in the way of proper debugging.
When running sh -X cert.sh , I am able to click retry, then "cancel" and get
into MSVC and get a stack :

MSVCRTD! 102137df()
MSVCRTD! 102135b1()
MSVCRTD! 1021353e()
PR_Free(void * 0x00de8e90) line 523 + 10 bytes
PORT_Free(void * 0x00de8e90) line 152 + 10 bytes
crlgen_destroyTempData(CRLGENGeneratorDataStr * 0x00de9490) line 1422 + 14 bytes
crlgen_updateCrl(CRLGENGeneratorDataStr * 0x00de9490) line 1467 + 9 bytes
yylex() line 133 + 12 bytes
CRLGEN_StartCrlGen(CRLGENGeneratorDataStr * 0x00de9490) line 170 + 5 bytes
UpdateCrl(CERTSignedCrlStr * 0x00de9778, PRFileDesc * 0x009a6730) line 490 + 9 bytes
GenerateCRL(NSSTrustDomainStr * 0x00dd10b0, char * 0x009a4a58, PRFileDesc *
0x009a6730, PRFileDesc * 0x00000000, char * 0x009a4ad8, int 0, char *
0x00000000, int 0, char * 0x00000000, int 0, int 0, char * 0x00000000,
secuPWData * 0x0012ff18, int 0) line 657 + 13 bytes
main(int 10, char * * 0x009a4970) line 992 + 61 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c59893d()

I believe this may be a case of a double-free.

Comment 2

[Julien Pierre]

FYI, this problem happens in the first CRL generation step in cert.sh .
I could not reproduce it on Solaris with dbx' librtc .

Jason, Christophe, this is probably a good reason to make the OBJD builds and
run our QA with USE_DEBUG_RTL=1 .

Comment 3

[Christophe Ravel]

Is it a bug specific to the OBJD build or does the OBJD build uncovered a
general issue ?

Comment 4

[Julien Pierre]

I don't believe the bug is specific to the OBJD build - OBJD uncovered a
pre-existing problem. OBJD means "use a debug runtime library", which is its
purpose, so it makes sense for us to run QA with it.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch (checked in)

Thanks a lot for getting a stack trace, Julien.

I found that this is not a double free, but rather a
heap buffer overflow.  In hindsight, the assertion
failure error message already said so:

    DAMAGE: after Normal block (#2692) at 0x0055CE50

The reason we overflow the buffer is that the size of
the buffer is incorrectly calculated.

http://lxr.mozilla.org/security/source/security/nss/cmd/crlutil/crlgen.c#1364

1364	 if (extStr->extData == NULL) {
1365	     extStr->extData = PORT_ZAlloc(MAX_EXT_DATA_LENGTH);
1366	     if (!extStr->extData) {
1367		 return SECFailure;
1368	     }
1369	 }

Since extStr->extData is an array of char * pointers, the
size should be MAX_EXT_DATA_LENGTH*sizeof(char *).  Or, as
Nelson would suggest, use PORT_ZNewArray(char *, MAX_EXT_DATA_LENGTH).

Comment 6

[Alexei Volkov]

Comment on attachment 186983 [details] [diff] [review]
Proposed patch (checked in)

Wan-teh,

Patch is correct. I've accidently replaced original correct allocation with
wrong macro.


From patch https://bugzilla.mozilla.org/attachment.cgi?id=180202

+     if (extStr->extData == NULL) {
+	  extStr->extData = (char**)PORT_ZAlloc(MAX_EXT_DATA_LENGTH *
sizeof(char*));
+	  if (!extStr->extData) {
+	      return SECFailure;
+	  }
+     }

Comment 7

[Wan-Teh Chang]

Attachment: Proposed patch v2

I found two other problems during code review.

The first is an off-by-one error.  In an array of
MAX_EXT_DATA_LENGTH elements, the allowed array
indices are 0,...,MAX_EXT_DATA_LENGTH-1.  So we
need to fail when extStr->nextUpdatedData equals
MAX_EXT_DATA_LENGTH.

The second is that it is possible for crlGenData->extensionEntry->extData
to be non-NULL and crlGenData->extensionEntry->nextUpdatedData to be 0
(if the PORT_Strdup call in crlgen_setNextDataFn_extension fails).
So it is better to test crlGenData->extensionEntry->extData than
crlGenData->extensionEntry->nextUpdatedData to determine whether
we need to free crlGenData->extensionEntry->extData.

Comment 8

[Wan-Teh Chang]

Comment on attachment 186983 [details] [diff] [review]
Proposed patch (checked in)

To prevent falling through the cracks, I checked in
this patch (which is the most important fix) on the
NSS trunk for NSS 3.10.1.

Checking in crlgen.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlgen.c,v  <--  crlgen.c
new revision: 1.2; previous revision: 1.1
done

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187072 [details] [diff] [review]
Proposed patch v2

These changes all look good to me.

Comment 10

[Wan-Teh Chang]

I checked in the rest of patch v2 on the NSS tip for
NSS 3.10.1.

Checking in crlgen.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlgen.c,v  <--  crlgen.c
new revision: 1.3; previous revision: 1.2
done

Comment 11

[Wan-Teh Chang]

*** Bug 302585 has been marked as a duplicate of this bug. ***

Comment 12

[Mikhail T.]

Can you, please, put out the patches, that were found worthy of being merged  
into  NSS_3_10_*, onto the FTP site next to the release tarball? 
 
Such practice would've saved a couple of evenings already and is sure to be 
appreciated by other users :-) 
 
Thanks! 

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Since the patch is available from a link on this page, putting an added copy
on an ftp server seems totally unnecessary.

Comment 14

[Mikhail T.]

=Since the patch is available from a link on this page, putting  
=an added copy on an ftp server seems totally unnecessary.  
  
Very unfortunate attitude... It is very useful to publish such  
patches in an obvious place -- such as next to the release tarballs.  
  
1) Without it, no one will know the bug exists (possibly creating corrupted  
certificates once in a while).  
2) Those who do notice the bug will not have to spend wonderful summer evenings  
chasing it -- as yours truly found himself doing.  
3) There will be no questions left about what OTHER bugs are already found,  
fixed, but not published other than in obscure depth of Bugzilla and the FUD 
implied by 1) will remain. 
  
Just look at the comments for Bug 302585 . Even (some) NSS maintainers did not 
know about the problem and the available fix. Do you honestly expect strangers 
to be able to find these fixes? 
 
I urge the NSS team once again to adopt the practice of publishing such patches 
in a prominent place. Right next to the release tarballs in the patches 
subdirectory seems most natural. 

Comment 15

[Julien Pierre]

Mikhail,

The way we deliver patches is through consolidated patch releases, not through
individual patch file for each bugs. We make new CVS tags available for those 
patch releases. To get the unreleased fixes for the latest release, you must
pull the source from the branch for that release, in this case NSS_3_10_BRANCH .
When someone needs patches, we cut a new tag and make a new patch release.
If you want the latest and greatest code, you pull the NSS tip (but this isn't
always guaranteed stable).

Individual patches are not easily managed, especially as fixes frequently affect
the same files and conflict. This is just the way the NSS team has chosen to
manage its releases. Over the years, this process has managed to meet the needs
of Netscape, AOL, iPlanet, Sun Microsystems, mozilla.org, and Red Hat .
Hopefully it should be able to meet yours as well.