115012 - 32K local allocation in mpprime.c

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alex Sverdlin]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:0.9.6) Gecko/20011127
BuildID:    2001112609

This bug is related to RSA key generation I think.
When I fill and submit the form above mozilla displays a boy where it generates
RSA key. During this process it craches. Here is pupup.log:
===

12-12-2001  01:13:40  SYS3175  PID 11c5  TID 0001  Slot 0073
D:\INET\OS2WEB\MOZILLA.EXE
c0000005
00b4fad9
P1=00000001  P2=0000001d  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=02d000ff  EBX=00b4fad4  ECX=00000000  EDX=00c80320
ESI=02d063b0  EDI=00000000  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:00b4fad9  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:0012f5f4  SSACC=f0f3  SSLIM=ffffffff
EBP=0012f618  FLG=00010206

MOZRMI36.DLL

------------------------------------------------------------

12-12-2001  07:24:47  SYS3175  PID 1931  TID 0004  Slot 0088
D:\INET\OS2WEB\MOZILLA.EXE
c0000005
575f4758
P1=00000001  P2=575f4758  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=00000006  EBX=01010001  ECX=1d462a29  EDX=00b5ff04
ESI=01010101  EDI=01010101  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:575f4758  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:00b5fee8  SSACC=f0f3  SSLIM=ffffffff
EBP=00010000  FLG=00010216

===

Reproducible: Always
Steps to Reproduce:
1.fill the form (check all boxes, fill something in textboxes)
2.submit
3.look at the key generation process

Actual Results:  crash

Expected Results:  proceed ;)

Comment 1

[Matthias Versen [:Matti]]

wfm with win2k build 20011212..
OS/2 only ?

Comment 2

[André Dahlqvist]

It generated fine for me with 2001-12-13-03 on Linux.

Reporter, please try a newer build.

Comment 3

[Olivier Cahagne]

wfm using build 2001121312 on Linux.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

->PSM, probably wfm.

Comment 5

[Stephane Saux]

worksforme. to qa.

Comment 6

[Alex Sverdlin]

No, I've tried the latest 2001121319 and the crash is still here:

12-15-2001  12:49:56  SYS3175  PID 004d  TID 0001  Slot 0074
D:\INET\OS2WEB\MOZILLA.EXE
c0000005
32353339
P1=00000001  P2=32353339  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=00e827d0  EBX=32353339  ECX=00000000  EDX=00e89e68
ESI=00e827d0  EDI=00000000  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:32353339  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:0012f834  SSACC=f0f3  SSLIM=ffffffff
EBP=0012f858  FLG=00010206

Comment 7

[Mike Kaply [:mkaply]]

I'll take for investigation.

I can't connect to this website.. does it still exist?

Comment 8

[Alex Sverdlin]

Yes, the site still works...

Comment 9

[Mike Kaply [:mkaply]]

Os/2 barfs on a 32K character string pointer as a local variable in mpp_make_prime.

Yes I said 32K. Who wrote this code?

The fix for Os/2 is to take the macintosh path which should be the default path.

Allocation 32K on the local heap is just bad.

We need this fix bad. We can't generate prime numbers without it.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Fix for problem

Have Os/2 go down the mac path - malloc 32K, don't get it from the local heap.

Comment 11

[Julien Pierre]

Wan-Teh,

This bug means that key generation won't work at all in mozilla 0.9.7 on OS/2,
making the new S/MIME support very hard to use (it works if you import keys, but
you just can't create new keys ...).

I just don't know if we have a proper mechanism to deliver this for 0.9.7 since
the browser tree is supposed to be closed soon, and we just tagged an NSS 3.3.2
last week. I can check the fix into NSS 3.4 tip but that won't help until
mozilla 0.9.8 .

Comment 12

[Wan-Teh Chang]

Comment on attachment 61922 [details] [diff] [review]
Fix for problem

This patch is fine. r=wtc.

Michael, if you want this fix in mozilla0.9.7,
could you get the appropriate approval?

Comment 13

[Wan-Teh Chang]

Assigned the bug to Nelson.

Nelson, is it possible to make the sieve array static
or allocate it from the heap for all platforms?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Allocating 32K from the stack is not "bad code", and works on all but
two platforms we support.  The code certainly could allocate this 
memory from the heap on all platforms.  

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I'll apply the submitted patch.
In what branches should it be applied?

Comment 16

[Wan-Teh Chang]

The default NSPR thread stack size is 64K on many platforms.
On OS/2, it is only 32K (see _MD_DEFAULT_STACK_SIZE in
mozilla/nsprpub/pr/include/md/_os2.h).  This problem can also
be fixed by increasing the default NSPR thread stack size to
at least 64K on OS/2.  This is a good thing to do because a
32K thread stack is pretty small.

Comment 17

[Mike Kaply [:mkaply]]

I will try your change, but the problem we were seeing was an issue with the 
stack probe (an internal compiler issue)

And as far as "bad code" vs. "good code", I would assert that putting a 32K 
variable in a function is at least not a very common thing to do.

In addition, I notice that a memset is used to clean up the allocation on mac, 
but nothing is used to clean the 32K stack entity, so theoretically pieces of 
the private key are still on the stack.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I believe there's no need to clear the 32K sieve after the operation is
done.  I believe the prime found with the sieve cannot be reconstructed 
from the sieve.

I'll check in the patch on the NSS_3_3_BRANCH (since that's where the 
NSS_CLIENT_TAG points, and no-one has requested anything else) and the 
trunk.  

Comment 19

[Wan-Teh Chang]

I agree that 32K is big for a stack variable.

There is performance advantage in using stack
variables, especially in multithreaded programs,
because that saves a trip to the heap.

Comment 20

[Julien Pierre]

Wan-Teh,

It sounds like in this case the heap contention is only occurring during RSA
keygen operation, right ? So it shouldn't critically affect performance for,
say, SSL servers. Maybe CMS would be affected but I doubt it. But for the client
malloc lock contention is probably not an issue.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Checked in revs 1.17 on trunk and 1.16.48.2 on NSS_3_3_BRANCH.
Someone else will need to move the NSS_CLIENT_TAG if mozilla is to 
take this change.

Comment 22

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.7

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Who is going to move the NSS_CLIENT_TAG??

Comment 24

[Julien Pierre]

I don't know, who has the power to do that ?

Comment 25

[Mike Kaply [:mkaply]]

WTC is absolutely correct. The core problem here was the OS/2 thread stack size.

I've noted this in http://bugzilla.mozilla.org/show_bug.cgi?id=115149

I still believe in the change that was made, but I wouldn't worry about the 
CLIENT_TAG right now.

The NSPR changes can take care of this.

Comment 26

[Asa Dotzler [:asa]]

So is this solved for 0.9.7? 

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

I believe the answer is: not until someone with authority to move the 
NSS_CLIENT_TAG tag on mpprime.c does so.

Comment 28

[Wan-Teh Chang]

I moved the NSS_CLIENT_TAG on mpprime.c.

I haven't checked in the fix on the MOZILLA_0_9_7_BRANCH.
Is it still okay to do that?

Comment 29

[Mike Kaply [:mkaply]]

I'd love to see it in 0.9.7 if it can go.

I'll put it in my Os/2 build whether it is in the 0_9_7 branch or not.

Comment 30

[Mike Kaply [:mkaply]]

Verify, this code fix is in.