502782 - socket.c test code has memory corruption which can cause execution to fail with SEGV on some OSes.

Status: NEW

(NSPR :: NSPR, defect)

Description

[Jonathan Leighton]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; en-us) AppleWebKit/525.28.3 (KHTML, like Gecko) Version/3.2.3 Safari/525.28.3
Build Identifier: 4.8.0

The socket.c test code contains memory corruption that has been observed to cause a SEGV failure on Solaris 5.10 and OpenSolaris 5.11 under certain conditions.  Specifically, if certain thread pointers are accessed with PR_GetThreadID(), the code will fail with SEGV.  PR_GetThreadID() is not called in socket.c, however adding it to socket.c can cause the code to fail due to the memory corruption.  (Reference 7/6/09 posting to mozilla.dev.tech.nspr newsgroup by Julien R. Pierre)

Reproducible: Always

Steps to Reproduce:
1.  On line 501 of socket.c, replace the thread variable t, with PR_GetThreadID(t).
2.  On Solaris 5.10 or OpenSolaris 5.11 run socket.c with the -d option.
3.  (This is just one of several of ways to exhibit the problem.)
Actual Results:  
The socket.c test code fails with SEGV reported.

Expected Results:  
Continue to run to completion as it does without the -d option.

It appears that (with pthreads enabled) socket.c's create_new_thread() routine is returning a thread pointer that only has local scope.  This is easily fixed.

Comment 1

[Jonathan Leighton]

Attachment: patch to fix memory corruption in socket.c

I've modified the create_new_thread() routine to return a pointer to memory allocated with malloc(), rather than a pointer to a local pthread_t variable.  The patch is based on the NSPR trunk.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 387140 [details] [diff] [review]
patch to fix memory corruption in socket.c

This patch leaks a block of memory for each created thread.  If that mitigates the problem, that may be a clue to the real underlying problem, but it is not a solution.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Also, the same code appears in two NSPR test files, socket.c and testfile.c.
If it needs to be fixed in one place, it probably also needs to be be fixed
in the other.  See
http://mxr.mozilla.org/security/search?string=create_new_thread

Comment 4

[Julien Pierre]

The problem is that native pthread_t and handles are being cast to PRThread* . This is clearly invalid. It doesn't affect the test program as written, since the value is only checked for NULL. But when trying to pass it to PR_GetThreadID, there is a crashed because that function tries to dereference the value, which is not a pointer.

I am wondering why we have those "native tests" calling native system thread functions here. Wan-Teh, any idea ? My preference would be for removing that part of the test. If we don't want to do that, then we should make a fix similar to attachment 387140 [details] [diff] [review] to allocate a real PRThread*. But then the question would be - how does that structure get filled properly ? I don't think it's really the job of the test program to do that, the code that creates a PRThread structure belongs in NSPR itself.

Comment 5

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.