f*ing pointer! Why doesn't this work?
Jerry Feldman
gaf at blu.org
Fri Jun 7 16:44:21 EDT 2002
I think your problem is here. I'll simplify;
Afunc(void *idata)
{
struct foo *data;
data = (struct foo *)idata;
:
free(data); /* This is a segfault*/
}
void *data;
data = malloc(...);
Afunc(&data);
Now, here is the way it should be:
Afunc(void *idata)
{
struct foo *data;
data = (struct foo *)*idata;
:
free(data); /* This woks nicely but ...*/
}
Since you are passing in a pointer to a pointer (double indirect), your
assignment is wrong:
data = (struct spawnData_s *)obj;
What you are assigning is a (struct spawnData_s **) not a (struct
spawnData_s *).
Actually, your code has several major problems (I assume that threadSpawn
is called for each thread you want to start).
1. don't use &data as your argument, just use data. The reason is that
in threadSpawn, data is an automatic variable and it's memor is
deallocated when the function returns. If the child thread continues to use
that pointer, several things could happen.
Another function in the parent is invoked and ovewrites that memory or
second call to threadSpawn overwrites it before your child thread finished
using it. This is a major bug.
2. The use of ThreadPool should be protected by a mutex. (I assume that
ThreadPool is initialized properly.(This is race condition).
ThreadPool[idx].Status is tested in threadSpawn. What if the child thread
should be switched before the child has a chance to set the Status. You
need to make sure you set the status BEFORE main or any other thread tests
it.
I suggest you make 2 corrections to your program:
1. add an element to ThreadPool for spawnData_s, then in threadSpawn,
ThreadPool[idx].data = (spawnData_s *)malloc(....).
2. pthread_create(&ThreadPool[idx].aThread, NULL, (void *)funcWrapper,
(void *)idx);
3. change void funcWrapper(void *obj) {
struct spawnData_s *data;
int idx;
data = (struct spawnData_s *)obj;
to:
void funcWrapper(void *obj) {
> struct spawnData_s *data;
> int idx;
/* You should pthread_mutex_lock here */
idx = (int)obj;
ThreadPool[idx].Status = tp_InUse;
data = ThreadPool[idx].data;
/* You may pthread_mutex_unlock here (or before setting data)*/
And in threadSpawn:
/* pthread_mutex_lock */
if (ThreadPool[idx].Status == tp_Available) {
ThreadPool[idx].data = malloc(...).
/* pthread_mutex_lock */
pthread_create(&ThreadPool[idx].aThread, NULL, (void *)funcWrapper,
(void *)idx);
Actually, this leaves a very slight open window, so I would add a
ThreadPool[idx].Status = InProcess; before unlocking the mutex in the
parent.
Much simpler. You eliminate the loop inside the thread, you eliminate the
confusion about the double indirect.
--
Jerry Feldman <gaf at blu.org>
Associate Director
Boston Linux and Unix user group
http://www.blu.org PGP key id:C5061EA9
PGP Key fingerprint:053C 73EC 3AC1 5C44 3E14 9245 FB00 3ED5 C506 1EA9
More information about the Discuss
mailing list