This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Implement checking for pthread_join


Hi,

IMHO checking (fortifying) should be implemented for pthread_*
functions, and here is my first attempt to do that for pthread_join.
I've recently found a bug in a program due to its inapropriate use of
pthread_join() [1], and neither gcc -fstack-protector [2], nor
-D_FORTIFY_SOURCE=2 could detect the bug.
The bug was that second param of pthread_join was pointing to an int,
rather than a (void*); and since the sizes of those are different on
x86-64 pthread_join overwrites the stack.

I tried to implement some checking for pthread_join, see attachments to
this e-mail, I also included a small test-program.

Compiling with a snapshot of gcc-4.3:
/usr/lib/gcc-snapshot/bin/gcc -Wall -W -O2 -D_FORTIFY_SOURCE=2 bugtest.c
-pthread
bugtest.c: In function 'worker':
bugtest.c:16: warning: unused parameter 'arg'
In function 'pthread_join',
    inlined from 'test_bug' at bugtest.c:6:
pthread2.h:17: warning: call to '__pthread_join_warn' declared with
attribute warning: pthread_join called with thread_return parameter
pointing to a location too small to fit a void* (possible buffer-overflow)

And of course running the test segfaults (if compiled with optimizations)
$ ./a.out
Segmentation fault
$ valgrind ./a.out
==7442== Memcheck, a memory error detector.
==7442== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==7442== Using LibVEX rev 1804, a library for dynamic binary translation.
==7442== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==7442== Using valgrind-3.3.0-Debian, a dynamic binary instrumentation
framework.
==7442== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==7442== For more details, rerun with: -v
==7442==
==7442== Jump to the invalid address stated on the next line
==7442==    at 0xFFFFFFFF: ???
==7442==    by 0x40061F: (within /tmp/a.out)
==7442==  Address 0xffffffff is not stack'd, malloc'd or (recently) free'd
==7442==
==7442== Process terminating with default action of signal 11 (SIGSEGV)
==7442==  Access not within mapped region at address 0xFFFFFFFF
==7442==    at 0xFFFFFFFF: ???
==7442==    by 0x40061F: (within /tmp/a.out)
==7442==
==7442== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 8 from 1)
==7442== malloc/free: in use at exit: 0 bytes in 0 blocks.
==7442== malloc/free: 2 allocs, 2 frees, 544 bytes allocated.
==7442== For counts of detected errors, rerun with: -v
==7442== All heap blocks were freed -- no leaks are possible.


[1] see: http://lkml.org/lkml/2008/1/4/93]
[2] Why gcc -fstack-protector can't detect this:
If I use gcc -fstack-protector-all, the additional code makes
pthread_join to overwrite the guard value, however since
it usually overwrites with 0 (return value of worker thread), no stack
overflow occurs/is detected with -fstack-protector-all.
If I change return NULL to return (void*)(-1), then gcc
-fstack-protector is able to detect the stack smashing.
However in the (common) case where return value  is NULL, gcc
-fstack-protector is not able to detect it, yet not using
-fstack-protector leads to a crash.


Best regards,
--Edwin
#include <sys/cdefs.h>
#if __USE_FORTIFY_LEVEL > 0 && defined __extern_always_inline
#if __USE_FORTIFY_LEVEL > 1
extern int __REDIRECT (__pthread_join_alias,
				(pthread_t th, void ** thread_return),
				pthread_join);
extern int __REDIRECT(__pthread_join_warn,
				(pthread_t th, void ** thread_return),
				pthread_join)
__warnattr("pthread_join called with thread_return parameter pointing to a location too small to fit a void* (possible buffer-overflow)");

__extern_always_inline
int pthread_join(pthread_t th, void **thread_return)
{
	if( thread_return != NULL && (__bos(thread_return) != (size_t)-1) &&
	    (__bos(thread_return) != sizeof(void*))) {
		return __pthread_join_warn(th, thread_return);
	}
	return __pthread_join_alias(th, thread_return);
}
#endif
#endif
#include <pthread.h>
#include "pthread2.h"
void test_bug(pthread_t id)
{
	int status;/* bug, should be 'void*' */
	pthread_join(id, (void**)&status);
}
void** unknown;
void test_ok(pthread_t id)
{
	void* status;
	pthread_join(id, (void**)&status);
	pthread_join(id, unknown);
}

void* worker(void* arg)
{
	return (void*)(NULL);
}

int main()
{
	pthread_t th, th2;
	pthread_create(&th, NULL, worker, NULL);
	pthread_create(&th2, NULL, worker, NULL);
	test_ok(th2);
	test_bug(th);
	return 0;
}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]