Bug 84970 – Mutex failure when HashTable is memcpy'ed in debug build

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yong Li]

In debug build, HashTable contains a mutex object. But Mutex isn't copyable on many platforms. However, JS Parser can potentially memcpy/memmove HashTable objects in its Vector<Scope>. When that happens, pthread_mutex_destroy can fail.

Probably we should change the way how Parser manages the scope vector (how about a linked list?).

For now, I'm going to make HashTable use OwnPtr<Mutex> for CHECK_HASHTABLE_ITERATORS, and assert the results of pthread_mutex_int() and pthread_mutex_destroy().

[Yong Li]

Created attachment 139023 [details]
the patch

[Joe Mason]

I notice that Mutex has WTF_MAKE_NONCOPYABLE, which is bypassed by memcpy/memmove.  Is there any way to assert in Vector<Scope> when a noncopyable object is added?  Preferably at compile time?

[Yong Li]

(In reply to comment #2)
> I notice that Mutex has WTF_MAKE_NONCOPYABLE, which is bypassed by memcpy/memmove.  Is there any way to assert in Vector<Scope> when a noncopyable object is added?  Preferably at compile time?

HashTable has copy ctor and operator= implemented, so it is copyable (mutex is not copied though)

[Build Bot]

Comment on attachment 139023 [details]
the patch

Attachment 139023 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12517992

[Joe Mason]

(In reply to comment #3)
> (In reply to comment #2)
> > I notice that Mutex has WTF_MAKE_NONCOPYABLE, which is bypassed by memcpy/memmove.  Is there any way to assert in Vector<Scope> when a noncopyable object is added?  Preferably at compile time?
> 
> HashTable has copy ctor and operator= implemented, so it is copyable (mutex is not copied though)

Yeah, that would make it much harder to detect.  Have you looked for other classes which may have this problem?

[Alexey Proskuryakov]

> Mutex isn't copyable on many platforms.

Are you saying that it's not movable with memcpy? Why does the problem occur?

[Yong Li]

(In reply to comment #6)
> > Mutex isn't copyable on many platforms.
> 
> Are you saying that it's not movable with memcpy? Why does the problem occur?

The behavior of using a copied mutex is undefined. (CRITICAL_SECTION on Windows is not copyable, too). The addresses of mutexes are critical, and can be used by system to identify them.

http://linux.die.net/man/3/pthread_mutex_init

"Only mutex itself may be used for performing synchronization. The result of referring to copies of mutex in calls to pthread_mutex_lock(), pthread_mutex_trylock(), pthread_mutex_unlock(), and pthread_mutex_destroy() is undefined."

[Yong Li]

Created attachment 139060 [details]
fix the build error

[Yong Li]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > I notice that Mutex has WTF_MAKE_NONCOPYABLE, which is bypassed by memcpy/memmove.  Is there any way to assert in Vector<Scope> when a noncopyable object is added?  Preferably at compile time?
> > 
> > HashTable has copy ctor and operator= implemented, so it is copyable (mutex is not copied though)
> 
> Yeah, that would make it much harder to detect.  Have you looked for other classes which may have this problem?

Not yet. The wild thing is JSC::Parser can memcpy HashSet, which is not very usual. The assert added for pthread_mutex_destroy could help detect similar issues.

[Alexey Proskuryakov]

Thank you for the explanation.

[Rob Buis]

Comment on attachment 139060 [details]
fix the build error

Looks good.

[WebKit Review Bot]

Comment on attachment 139060 [details]
fix the build error

Clearing flags on attachment: 139060

Committed r115988: <http://trac.webkit.org/changeset/115988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mario Sanchez Prada]

This patch has caused the GTK 64bit Debug bot to fail for a while already [1], with the following error:

*** glibc detected *** /home/slave/webkitgtk/gtk-linux-64-debug/build/Tools/gtk/../Scripts/../../WebKitBuild/Debug/Programs/TestWebKitAPI/WTF/TestHashMap: malloc(): memory corruption: 0x000000000067fdf0 ***

I've investigated a bit the issue and localized the source of this problem in this patch, specifically in the changes done in HashTable.h.

I will try to come up with a solution for this, but will appreciate any hint on this in case you have any.

Here's a new bug for tracking down this issue: https://bugs.webkit.org/show_bug.cgi?id=88419

Thanks,
Mario

[1] http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/33848/steps/API%20tests/logs/stdio