How do I copy this temporary char* variable into my struct for more permanent storage?

168 Views Asked by At

I'm trying to implement a thread pool, and I'm having quite the bit of trouble with getting the filepaths I'm supposed to work with to be more permanently stored than the temporary instance they're existing inside the OnOpen function from ftw. I'm not allowed to do malloc for every filepath that gets handled.

This is what I have currently to try and get OnOpen to not give temporary data to my threads, and I am confused as to the reason for it to crash on memcpy.

I'd love to know how to get the data to be secure from a temp variable being edited without creating a superfluous array of char* that I perform memcpy with.

typedef struct Task{
    void (*taskFunc)(char*);
    char* arg;
} Task;

void HashFunc(char* arg)
{
    pthread_mutex_lock(&lock);
    printf("%s\n", arg);
    thingsDone++;
    pthread_mutex_unlock(&lock);
}

static int OnOpen(const char* path, const struct stat* sb, int flags)//will send folder paths too
{
    if(strstr(path, ".exe") || strstr(path, ".cfg")) return 0;
    Task t = {
        .taskFunc = &HashFunc,
        .arg = path
    };
    memcpy(t.arg, path, strlen(path));
    while(taskCount == MAX_OPEN_FILE_HANDLES-1); //busy wait
    submitTask(t);
    return 0;
}

Edit: Great feedback, but it's not quite what I need.

Here below I'll add some stuff that is related to the code that I'm having trouble with, namely the threadpool and how I use the task struct, which might be useful in getting a better understanding of what can be done and how I can go about it:

void executeTask(Task* task) {task->taskFunc(task->arg);}

void* threadFunc(void* arg)
{
    Task task;
    while (!(doneSending==1 && thingsDone == thingsToDo))
    {
        pthread_mutex_lock(&lock);
        while (taskCount==0 && doneSending==0) {pthread_cond_wait(&condQueue, &lock);}
        task = taskQueue[0];
        for (int i = 0; i < taskCount-1; i++) {taskQueue[i] = taskQueue[i+1];}
        taskCount > 0 ? --taskCount : 0;
        pthread_mutex_unlock(&lock);
        if (doneSending==0 || thingsDone<thingsToDo) executeTask(&task);
        printf("%d, %d, %d, %d\n", taskCount, thingsDone, thingsToDo, doneSending);
    }
}

void submitTask(Task task)
{
    pthread_mutex_lock(&lock);
    taskQueue[taskCount++] = task;
    ++thingsToDo;
    pthread_mutex_unlock(&lock);
    pthread_cond_signal(&condQueue);
}

My threadpool consists of 8 threads, which is also the size of my taskQueue.

I used to have .arg = strcpy(temp, path) but as temp is temporary, I got bad data printed in the hashFunc.

Each thread is supposed to have its own copy of the Task struct to work with, so as to not risk them interfering with eachother.


Final Edit: I got it working, here is how it needed to look:

volatile int taskIdx = 0, pathIdx = 0;

Task taskArray[MAX_OPEN_FILE_HANDLES];
char* pathQueue[MAX_OPEN_FILE_HANDLES];

void* threadFunc(void* args)
{
    Task task;
    while (!(doneSending==1 && taskIdx == pathIdx))
    {
        if (doneSending && taskIdx==pathIdx) break;
        pthread_mutex_lock(&lock);
        pthread_cond_wait(&condArray, &lock);
        if (doneSending && taskIdx==pathIdx)
        {
            pthread_mutex_unlock(&lock);
            break;
        }
        task = taskArray[taskIdx];
        taskIdx = (taskIdx+1)%MAX_OPEN_FILE_HANDLES;
        pthread_mutex_unlock(&lock);
        executeTask(&task);
    }
}

void submitTask(Task t)
{
    pthread_mutex_lock(&lock);
    taskArray[pathIdx] = t;
    pathIdx = (pathIdx+1)%MAX_OPEN_FILE_HANDLES;
    pthread_cond_signal(&condArray);
    pthread_mutex_unlock(&lock);
}

static int OnOpen(const char* path, const struct stat* sb, int flags)
{
    if(flags != FTW_F || strstr(path, ".cfg") || strstr(path, ".exe") || strstr(path, ".vscode") || strstr(path, "anticheat") || strstr(path, "Makefile")) return 0;
    if (thingsToDo-thingsDone == MAX_OPEN_FILE_HANDLES) HashFunc((char*)path, thingsToDo++);
    else
    {
        Task t = {
            .taskFunc = &HashFunc,
            .filePath = strcpy(pathQueue[pathIdx], path)
        };
        submitTask(t);
    }
    return 0;
}

int main()
{
    pthread_mutex_init(&lock, NULL);
    pthread_cond_init(&condArray, NULL);
    for (int i=0; i<MAX_OPEN_FILE_HANDLES; i++)
    {
        if(pthread_create(&pool[i], NULL, &threadFunc, NULL) != 0) perror("pth_create");
        pathQueue[i] = calloc(MAX_PATH_LENGTH, sizeof(char));
    }
2

There are 2 best solutions below

2
On

First.

memcpy(t.arg, path, sizeof(path));

This operation is deeply wrong. sizeof(path) returns a size of a pointer variable. It is very unrelated to length of the string. Use strcpy.

Just put array of chars into struct Task. Every thread would have it's own copy without interfering with other threads:

typedef struct Task{
    void (*taskFunc)(char*);
    char arg[PATH_MAX];
} Task;
strcpy(Task.arg, path);

Edit

If each thread is expected to have its own copy of arg (not whole Task) just use strdup(). It's far more handy than strlen()/malloc()/strcpy() sequence.

    Task t = {
        .taskFunc = &HashFunc,
        .arg = strdup(path),
    };

Remeber to release the memeory with free(t.arg) when thread finishes it's job.

0
On
  1. You do not allocate any memory to store the path
  2. sizeof(path) is giving you length of the pointer not the length of the path. To get length of the string use strlen
  3. memcpy(t.arg, path, sizeof(path)); copies only the number sizeof(char *) bytes, not the the whole string .
    size_t arglen = strlen(path);
    Task t = {
        .taskFunc = &HashFunc,
        .arg = malloc(arglen + 1),
    };
    
    /* add some error checking */
    strcpy(t.arg, path);