We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 601

Summary: SDL thread join frees first some data and later a thread tries to read same data
Product: SDL Reporter: Pauli Nieminen <paniemin>
Component: threadAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: renesd, sezeroz
Version: 1.2.13Keywords: target-1.2.14
Hardware: x86   
OS: Linux   
URL: http://www.wesnoth.org

Description Pauli Nieminen 2008-07-01 15:21:37 UTC 
Valgrind reports memory write after free in SDL thread code:
==28062== Thread 2:
==28062== Invalid write of size 4
==28062==    at 0x405F1AE: (within /usr/lib/libSDL-1.2.so.0.11.1)
==28062==    by 0x40ABBBC: (within /usr/lib/libSDL-1.2.so.0.11.1)
==28062==    by 0x43C8FD9: start_thread (pthread_create.c:297)
==28062==    by 0x42FC93D: clone (in /usr/lib/debug/libc-2.7.so)
==28062==  Address 0x4583a88 is 8 bytes inside a block of size 792 free'd
==28062==    at 0x402165C: free (vg_replace_malloc.c:323)
==28062==    by 0x405F303: SDL_WaitThread (in /usr/lib/libSDL-1.2.so.0.11.1)
==28062==    by 0x807011D: threading::thread::join() (thread.cpp:74)
==28062==    by 0x807013A: threading::thread::~thread() (thread.cpp:60)
==28062==    by 0x8080CD5: (anonymous namespace)::process_queue(void*) (network_worker.cpp:598)
==28062==    by 0x405F1AA: (within /usr/lib/libSDL-1.2.so.0.11.1)
==28062==    by 0x40ABBBC: (within /usr/lib/libSDL-1.2.so.0.11.1)
==28062==    by 0x43C8FD9: start_thread (pthread_create.c:297)
==28062==    by 0x42FC93D: clone (in /usr/lib/debug/libc-2.7.so)

Here is websvn links to code which was used to generate valgrind report:
http://svn.gna.org/viewcvs/wesnoth/trunk/src/network_worker.cpp?rev=27624&view=markup
http://svn.gna.org/viewcvs/wesnoth/trunk/src/thread.hpp?rev=23842&view=markup
http://svn.gna.org/viewcvs/wesnoth/trunk/src/thread.cpp?rev=27646&view=markup

This report can be easily reproduced using svn trunk unit tests:
scons test build=debug
valgrind ./test-debug

I'm suspecting that this valgrind error might be source for some random crashes in wesnoth.

Athlon XP-M processor.
ubuntu hardy 8.04. 
SDL 1.2.13 binary package.
Comment 1 Ryan C. Gordon 2009-09-13 16:33:23 UTC 
Tagging this bug with "target-1.2.14" so we can try to resolve it for SDL 1.2.14.

Please note that we may choose to resolve it as WONTFIX. This tag is largely so we have a comprehensive wishlist of bugs to examine for 1.2.14 (and so we can close bugs that we'll never fix, rather than have them live forever in Bugzilla).

--ryan.
Comment 2 Sam Lantinga 2009-09-21 01:51:33 UTC 
Ryan, can you look at this for 1.2 and 1.3?  Thanks!
Comment 3 Rene Dudfield 2009-09-22 13:54:34 UTC 
hello,

I think a fix might be for SDL_ThreadsQuit to use the thread_lock like the rest of the functions.

Here's what I think is the problem...

It's possible for a SDL_DelThread to put the threads in a state where SDL_ThreadsQuit is called(after SDL_DelThread has released thread_lock).  However SDL_ThreadsQuit does not use thread_lock, which means another thread being created would have weird issues happening.

Imagine this sequence:

SDL_DelThread (acquires lock and gets to work)
SDL_AddThread (waits for lock)
SDL_DelThread (almost finishes work, and releases lock)
SDL_AddThread (acquires lock and gets to work)
SDL_DelThread (calls SDL_ThreadsQuit whilst AddThread is going)
SDL_ThreadsQuit (sets thread_lock to NULL)
... weirdness, where multiple threads can mess with the data structures.



cheers,
Comment 4 Sam Lantinga 2009-09-22 23:38:24 UTC 
This might be fixed by the latest checkin:
http://www.libsdl.org/tmp/SDL-1.2.zip

Can you try it out and let me know if that takes care of the issue?

Thanks!
Comment 5 Sam Lantinga 2009-10-10 01:13:39 UTC 
Tossing to Ryan for verification.
Comment 6 Ryan C. Gordon 2009-10-10 09:54:44 UTC 
I couldn't trigger this with the wesnoth unit tests, even testing with a revision before the fix.

It would be useful if Pauli could retest and let us know if the issue is resolved.

--ryan.
Comment 7 Sam Lantinga 2009-10-21 20:44:07 UTC 
E-mail to the author of this bug is bouncing.  I believe this is fixed so I'm going to close this bug.  Please re-open it if it's still active in SDL 1.3!