8112 - Lack of thread-safety with JS Math routines

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Scott Furman]

The variable 'errno' is not protected against thread races even when
JS_THREADSAFE is defined.  errno is used in jsnum.c, jsdtoa.c, and fdlibm/*.c.

This might cause Math errors on one thread to be inappropriately visible
to another thread.

Comment 1

[Michael Ang]

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

Comment 2

[Scott Furman]

mang ducked out on this bug and gave it to me - it looks like you inherit it now

Comment 3

[Mike McCabe]

Ducking out as well.  Roger, you're the lucky one.

Comment 4

[leger]

Setting all Javacript bugs to rginda QA Contact.

Comment 5

[Phil Schwartau]

Reassigning to Patrick as per meeting -

Comment 6

[Patrick C. Beard]

Reassigning to mitesh.

Comment 7

[Mitesh Shah]

In jsnum.c, the variable 'errno' is used to check the overflow during the 
js_strtod call. Instead of using the global variable we can pass a variable to 
the function JS_strtod (defined in jsdtoa.c line: 1079). Since it's 
JS_FRIEND_API function, does anyone think that we can change the function 
interface? 

Comment 8

[Mitesh Shah]

As per the discussions, JS_strdtoa is JS_FRIEND_API so we can add one parameter 
to rport overflow and get rid of global variable errno altogether. 

Regarding k_standard.c, instead of reporting error through errno, we can use use 
fputs to communicate the error to the user.

Now the only place remaining is jsscan.c where we report an error encounterd 
during fopen call using the errno. In the worst case, the error reported would 
not be true (if another thread sets the errno to some different value). I 
recommend, instead of using the strerror(errno), we can put a static string "No 
such file of directory" message.

Comment 9

[Mike McCabe]

> Regarding k_standard.c, instead of reporting error through errno,
> we can use use fputs to communicate the error to the user.

I don't think this is appropriate for an embedded engine; it would be best to
communicate failure through the calling mechanism and let the caller work around
it or register a javascript exception.

Using a static string in the file case sounds fine to me.

Comment 10

[Mitesh Shah]

That's a good suggestion. Which is basically the same solution as jsnum.c. 
Patrick also suggested the same way. Callers of __kernel_standard function are 
not actually checking the return value of errno but the changes will be helpful 
in the future if it is required.

Comment 11

[Mitesh Shah]

Attachment: Code Changes in patch format.

Comment 12

[Patrick C. Beard]

Retargeting to 0.8.1.

Comment 13

[chris hofmann]

I think this missed the 0.8.1 train.  lets to it early in 0.9

Comment 14

[Patrick C. Beard]

Let's get some reviewers, it's a really simple change.

Comment 15

[Brendan Eich [:brendan]]

Some funky indentation (tab expansion? good if so) and stylistic violations (int
err=0; is not how JS code initializes vars -- spaces on each side of = are more
the ticket).  Other than that, looks good to me.  Who will check this in?

/be

Comment 16

[Patrick C. Beard]

I would be happy to clean up and check in the patch.

Comment 17

[Brendan Eich [:brendan]]

OK, I think you have r=beard and sr=me on mitesh's patch, with cleanups.

/be

Comment 18

[Patrick C. Beard]

Attachment: cleaned up, removed extraneous assignments to err

Comment 19

[Patrick C. Beard]

Fix checked in.

Comment 20

[Phil Schwartau]

Marking Verified - 

Comment 21

[Phil Schwartau]

Strange: I can compile the JS shell on WinNT now, but not on Linux:

gcc -o Linux_All_DBG.OBJ/k_standard.o -c -Wall -Wno-format -DGCC_OPT_BUG 
-DXP_UNIX -g -DXP_UNIX -DSVR4 -DSYSV -D_BSD_SOURCE -DPOSIX_SOURCE -DX86_LINUX  
-DDEBUG -DDEBUG_pschwartau -I.. -I../Linux_All_DBG.OBJ -DJSFILE  -D_IEEE_LIBM 
k_standard.c
k_standard.c:125: conflicting types for `__kernel_standard'
fdlibm.h:264: previous declaration of `__kernel_standard'
make[1]: *** [Linux_All_DBG.OBJ/k_standard.o] Error 1
make[1]: Leaving directory `/data/phil/JS_TRUNK/mozilla/js/src/fdlibm'
make: *** [all] Error 2



In js/src/fdlibm/k_standard.c
------------------------------------------------------------------
#ifdef __STDC__
	double __kernel_standard(double x, double y, int type) 
#else
	double __kernel_standard(x,y,type) 
	double x,y; int type;
#endif
------------------------------------------------------------------


In js/src/fdlibm/fdlibm.h
------------------------------------------------------------------

/* fdlibm kernel function */
extern double __kernel_standard __P((double,double,int));

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



I'm running egcs-2.91.66.
I've had weird problems lately compiling on my Linux box lately. 
Is that the likely cause here, or is gcc right to error on this? 

Comment 22

[Mike Shaver (:shaver emeritus)]

I'm seeing the same problem, and it sure looks like a bug to me.  I can't
understand how the tinderboxes don't see it.  Can we get a fix for this?

(Reopening.)

Comment 23

[Mike Shaver (:shaver emeritus)]

Index: fdlibm.h
===================================================================
RCS file: /cvsroot/mozilla/js/src/fdlibm/fdlibm.h,v
retrieving revision 1.8
diff -u -r1.8 fdlibm.h
--- fdlibm.h	2001/03/09 04:07:15	1.8
+++ fdlibm.h	2001/04/19 15:20:27
@@ -261,7 +261,7 @@
 #endif
 
 /* fdlibm kernel function */
-extern double __kernel_standard __P((double,double,int));	
+extern double __kernel_standard __P((double,double,int,int *));	
 extern double __kernel_sin __P((double,double,int));
 extern double __kernel_cos __P((double,double));
 extern double __kernel_tan __P((double,double,int));

This WORKSFORME.

For 0.9 to fix a build blocker? Please?

Comment 24

[Brendan Eich [:brendan]]

Yes, let's fix this in the trunk, now (0.9).  I failed to test the patches here.
Beard, how did this work on any platform?

OTOH, k_standard.c doesn't seem to be compiled when I build js as part of
Mozilla.  It's in fdlibm/Makefile.ref's list of C source files, but not in
fdlibm/Makefile.in's list.  So this seems to be only a JSRef issue.

/be

Comment 25

[Patrick C. Beard]

This patch looks good. I verified that Makefile.ref builds with it. r=beard

Comment 26

[Brendan Eich [:brendan]]

sr=[e-mail] (nit: no space between int and * in that final parameter
type, to match the ugly prevailing style).

I think I can a= this too, it should not affect non-ref builds.  I'll just check
it in, if that's ok with everyone.

/be

Comment 27

[Patrick C. Beard]

I agree, check it in.

Comment 28

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 29

[Phil Schwartau]

Verified - I can now compile the JS shell on both WinNT and Linux.