282994 - Crash in debug build if I press submit button. Problem is with PR_dtoa function on ARM without FP unit.

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Tom Marn]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050110 Firefox/1.0 (Debian package 1.0+dfsg.1-2)
Build Identifier: CVS Source : Stable Mozilla 1.7.5 - Released December 17, 2004

Problem is with a PR_dtoa function in mozilla/nsprpub/pr/src/misc/prdtoa.c
Converting from double to string produce memory leak. Minimo crash will with
Assertion failure: lock != NULL, at ptsynch.c:206

 PR_dtoa is quite broken on ARM platform (PXA255-400Mhz version without FP
unit), Minimo (from mozilla source 1.7.5) is compiled with softfloat compiler
(gcc version 3.3.3/glibc 2.3.2, from www.kegel.org).

The reason is because ARM platform (little endian) use (mixed endian) when
manipulating with float numbers. Maybe the prdtoa.c functions are not compliant
with that arhitecture.

I also found the same problem in
http://lists.debian.org/debian-arm/2003/10/msg00028.html

Reproducible: Always

Steps to Reproduce:
1. build debug version of Minimo. (release version will skip assert checking and
"submit" will not be executed and not action forward to server :( ).
2. ./Minimo www.google.com
3. Enter something "test" into search textbox
4. Press "Google search"
5. Nothing happend after submit in release version.
   Crash in debug version: Assertion failure: lock != NULL, at ptsynch.c:206
Actual Results:  
Because pow5mult called from PR_dtoa is extremly slow you will got an error
after 1 minute:
Assertion failure: lock != NULL, at ptsynch.c:206



Expected Results:  
Function should convert double value to string, returns and than submit the
entered text forward (I guess).


Look into prdtoa.c: 
function static Bigint *mult(CONST Bigint *a, CONST Bigint *b) which is called
from pow5mult generates list with numbers of multiply of 5. 
Set breakpoint into line 678 of mult function and print return c variable ,
The table which generates here must have numbers ( 5 25 125 625 3125 15626 ... }

gdb: print *c

Next nubmer which is generated is 78125, which is OK  ( 5 * 15625)
OK $2 = {next = 0xffffffff, k = 1, maxwds = 2, sign = 0, wds = 1, x = {78125}}

Next nubmer is 390625, OK too. ( 5 * 78125)
$4 = {next = 0x0, k = 1, maxwds = 2, sign = 0, wds = 1, x = {390625}}

Than instead of 1953125 value of x is {2264035265}, IS THIS WRONG ????
$5 = {next = 0xffffffff, k = 1, maxwds = 2, sign = 0, wds = 2, x = {2264035265}}

gdb: bt

#0  mult (a=0x1f88c0, b=0x1f88c0) at prdtoa.c:678
#1  0x404933a4 in pow5mult (b=0x1f8900, k=77644143) at prdtoa.c:739
#2  0x40497294 in $a () at prdtoa.c:2341

Before the assertion happens in ptsynch.c:206 call stack is:

(gdb) bt
#0  PR_Unlock (lock=0x0) at ptsynch.c:206
#1  0x404926e4 in Balloc (k=16) at prdtoa.c:418
#2  0x40492ea0 in mult (a=0x424ab008, b=0x424ab008) at prdtoa.c:620
#3  0x404933a4 in pow5mult (b=0x424cc008, k=2369) at prdtoa.c:739
#4  0x40497294 in $a () at prdtoa.c:2341

Also, maybe something is wrong (Kmax) in Balloc function which generates
freelist[k] table with pointers on allocated memory where converted numbers will
be stored. (guess)

Comment 1

[Wan-Teh Chang]

Tom, could you find out the CVS revision of
prdtoa.c?  You can find that out with the
following commands:

  cd mozilla/nsprpub/pr/src/misc
  cvs status prdtoa.c

In the current version of prdtoa.c, we have
the following code that tries to handle ARM:

#if defined(__arm) || defined(__arm__) || defined(__arm26__) \
    || defined(__arm32__)
#define IEEE_ARM
#elif defined(IS_LITTLE_ENDIAN)
#define IEEE_8087
#else
#define IEEE_MC68k
#endif

This code was added by Darin Fisher to fix bug 209814.
Does your prdtoa.c have this code?  Could you find
out if the above code causes IEEE_ARM to be defined
in your environment?

Comment 2

[Tom Marn]

(In reply to comment #1)
> Tom, could you find out the CVS revision of
> prdtoa.c?  You can find that out with the
> following commands:
> 
>   cd mozilla/nsprpub/pr/src/misc
>   cvs status prdtoa.c
> 
> In the current version of prdtoa.c, we have
> the following code that tries to handle ARM:
> 
> #if defined(__arm) || defined(__arm__) || defined(__arm26__) \
>     || defined(__arm32__)
> #define IEEE_ARM
> #elif defined(IS_LITTLE_ENDIAN)
> #define IEEE_8087
> #else
> #define IEEE_MC68k
> #endif
> 
> This code was added by Darin Fisher to fix bug 209814.
> Does your prdtoa.c have this code?  Could you find
> out if the above code causes IEEE_ARM to be defined
> in your environment?
> 

Hi, CVS Revision status of prdtoa.c that you ask for is:

File: prdtoa.c          Status: Up-to-date

   Working revision:    3.7.4.4
   Repository revision: 3.7.4.4 /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
   Sticky Tag:          MOZILLA_1_7_5_RELEASE (revision: 3.7.4.4)
   Sticky Date:         (none)
   Sticky Options:      (none)


#define's are there but nothing helps, i also try with the newest 1.8.X
(11.feb.2005), but problem still remains.

File: prdtoa.c          Status: Up-to-date

   Working revision:    3.7.4.7
   Repository revision: 3.7.4.7 /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
   Sticky Tag:          NSPRPUB_PRE_4_2_CLIENT_BRANCH (branch: 3.7.4)
   Sticky Date:         (none)
   Sticky Options:      (none)

Just ask, I will check anything to solve this problem.

Comment 3

[Wan-Teh Chang]

Tom: thanks for the cvs revision info.
Your file (rev. 3.7.4.4, MOZILLA_1_7_5_RELEASE)
has Darin's fix.

Could you confirm that prdtoa.c defines
IEEE_ARM?  (which means one of the following
macros: __arm, __arm__, __arm26__, __arm32__
is defined)

If so, this means Darin's fix does not address
your problem.  In that case I'll need to have
Doug Turner take on this bug.

Comment 4

[Wan-Teh Chang]

Tom:

Specifically, please perform the following experiment:

Edit prdtoa.c and add the following line

  #error "IEEE_ARM is defined"

after the line

  #define IEEE_ARM

and then go to the nsprpub/pr/src/misc directory
in your build tree and do a "make".  See if the
compilation succeeds or fails.

Comment 5

[timeless]

alternatively, could you attach the file generated by:
make prdtoa.i (make from the directory that has prdtoa.o)

Comment 6

[Tom Marn]

(In reply to comment #4)
> Tom:
> 
> Specifically, please perform the following experiment:
> 
> Edit prdtoa.c and add the following line
> 
>   #error "IEEE_ARM is defined"
> 

Yes it fails after make, I confirming that IEEE_ARM define is use.

prdtoa.c:138:2: #error IEEE_ARM
make: *** [prdtoa.o] Error 1

Comment 7

[Tom Marn]

Attachment: gziped prdtoa.i file.

Comment 8

[Tom Marn]

Attachment: gziped prdtoa.i file.

Comment 9

[Tom Marn]

Attachment: source code to reproduce the same error.

This is the code (see attachment) for reproduce the same error. 
Just wait 30 seconds and you will get the same assertion as in the mozilla
minimo browser.
Assertion failure: lock != NULL, at ptsynch.c:206
Aborted

copy code into mozilla/nsprpub/pr/tests/testdtoa.c 
and change Makefile (see testdtoa.c for details).

Comment 10

[Tom Marn]

Attachment: This is a patch for nspr detection workaround on ARM

I found the location where PR_dtoa crashes. It is in a whatnspr.c file 
set_whatnspr function. Instead of returning from PR_dtoa with -1 value function
dies. Patch skips fault version detection of nspr library and forces to setup
correct nspr version with r = 0. See attached patch.
tom

Comment 11

[Tom Marn]

Attachment: Another patch to workaround a dtoa on ARM

Replacing dtoa with sprintf in nsStringObsolite.cpp:Modified_cnvtf(char *buf,
int bufsz, int prcsn, double fval) method. Note that this is just a workaround
until dtoa is not fixed.
tom

Comment 12

[Doug Turner (:dougt)]

I am not seeing this problem on either Linux ARM or on WinCE ARM.  

Comment 13

[Tom Marn]

(In reply to comment #12)
> I am not seeing this problem on either Linux ARM or on WinCE ARM.  

The problem is with Linux ARM without FPU (tested on PXA255) and Minimo compiled
with softfloat compiler (for faster code) and with disabled kernel FastFPU
emulation of course.
I did workaround hacks, i can send you patches which work without crashing on my
platform.
The problem was with functions that call PR_dtoa.

Comment 14

[Tom Marn]

Attachment: dtoa and jsdtoa workaround for ARM linux

PR_dtoa workaround (with debug printf's, just to see fixed locations.)

Comment 15

[Wan-Teh Chang]

Comment on attachment 190515 [details] [diff] [review]
dtoa and jsdtoa workaround for ARM linux

Tom, thanks for the patch.

I will attach a better patch for
mozilla/security/nss/lib/base/whatnspr.c.
In short, the code that's causing problem for
ARM in that file is obsolete, so the better fix
is to simply delete that code.

Your changes for the other files in this patch
have two problems:
- You use the C++ comment delimiter //, which is
  not supported by all C compilers yet.
- You need to code these changes with ifdefs so
  that we continue to use the original code on
  other platforms.

Comment 16

[Wan-Teh Chang]

Tom, I decided to open bug 317052 to deal with
mozilla/security/nss/lib/base/whatnspr.c, so the
patch for that file will be in that bug report.

Comment 17

[Tom Marn]

(In reply to comment #16)
> Tom, I decided to open bug 317052 to deal with
> mozilla/security/nss/lib/base/whatnspr.c, so the
> patch for that file will be in that bug report.
> 

Ok, I agree. I sent previous patches just for demonstration purpose where the bugs occured, and how to overcome the problems. I didn't pay attention on coding syntax. The patches were sent because of Doug Turner comment #12.

Comment 18

[Eric Tsai]

(In reply to comment #14)
> Created an attachment (id=190515) [edit]
> dtoa and jsdtoa workaround for ARM linux
> 
> PR_dtoa workaround (with debug printf's, just to see fixed locations.)

Tom, I have some question about the patch of hack4 in jsnum.c

+	double num = (jsdouble)d;
+	char temp[255];
+	printf("DOUBLE:%f\n",num);
+	sprintf(temp,"%f",(int)num);

num will be casted to integer, if num is not an integer, like 3.5, cast will create 0.0.
why not just use sprintf(temp,"%f",num) instead?

+	printf("CONVERTED:%s\n", temp);
+	numStr = temp;

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

eh? No, that would just truncate the value, yielding 3. but printing that as a %f is probably not such a good idea...

Comment 20

[Eric Tsai]

(In reply to comment #19)
> eh? No, that would just truncate the value, yielding 3. but printing that as a
> %f is probably not such a good idea...
> 

It's strange, when I use TestGtkEmbed to open http://maps.google.com , the message makes me confused:

== start of message ==

jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=3.500000)
DOUBLE:3.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=0.000000)
DOUBLE:0.000000
CONVERTED:0.000000

== End of messages ==

This is the reason I remove the cast of num in hack 4. But even the convert look correct, my google map also can't show anything :-(

Comment 21

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 22

[Wayne Mery (:wsmwk)]

Reporter is gone.

Is there a way to determine whether this issue still exists?

Comment 23

[Kai Engert [:KaiE:]]

Are there any recent crash reports pointing to PR_dtoa ?

Comment 24

[Wayne Mery (:wsmwk)]

a few version 78

• https://crash-stats.mozilla.org/signature/?proto_signature=~PR_dtoa&product=Thunderbird&signature=__pthread_kill%20%7C%20abort%20%7C%20gpusGenerateCrashLog.cold.1&date=%3E%3D2021-09-04T23%3A49%3A00.000Z&date=%3C2021-10-04T23%3A49%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#summary
• https://crash-stats.mozilla.org/signature/?proto_signature=~PR_dtoa&product=Thunderbird&signature=__pthread_kill%20%7C%20pthread_kill%20%7C%20abort%20%7C%20gpusGenerateCrashLog.cold.1&date=%3E%3D2021-09-04T23%3A49%3A00.000Z&date=%3C2021-10-04T23%3A49%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports
• bp-811a6fa7-aba5-48c0-861b-ca5da0210920

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:KaiE, could you have a look please?

For more information, please visit BugBot documentation.