thread race condition accessing axl_kvtrees[id]

[rhaas80]

In

https://github.com/ECP-VeloC/AXL/blob/a799cd9b7a406a80173ecb2bf3a32000f432f8ce/src/axl_pthread.c#L321

axl_kvtrees is accessed without protecting from race conditions.

Looking eg at https://github.com/ECP-VeloC/AXL/blob/a799cd9b7a406a80173ecb2bf3a32000f432f8ce/src/axl.c#L98

where one has

   pthread_mutex_lock(&id_lock);

    int id = axl_kvtrees_count;
    axl_kvtrees_count++;

    axl_kvtrees = realloc(axl_kvtrees, sizeof(struct kvtree*) * axl_kvtrees_count);
    axl_kvtrees[id] = new;

    pthread_mutex_unlock(&id_lock);

it seems clear that axl_kvtress is accessed in a multi-threaded context. Since realloc can move the block of data when allocating memory ie in new_ptr = realloc(old_ptrs, new_size) there is no guarantee that new_ptr == old-ptr one must not assume that axl_kvtrees[id] is accessible without the lock.

[tonyhutter]

Good catch! We should add a wrapper function that gets the kvtree and handles the locking, like:

- kvtree* file_list = axl_kvtrees[id]; 
+ kvtree* file_list = get_kvtree(id);

[rhaas80]

If there is no objection I will try and give adding correct locking code, using a get_kvtree function plus locking for access to the other global variables a try.

[rhaas80]

Branch https://github.com/rhaas80/AXL/tree/rhaas/locks contains an implementation protecting against the race condition. Changes are: https://github.com/rhaas80/AXL/compare/rhaas/keyvalue...rhaas80:rhaas/locks

Implementation notes:

• this protects only axl_kvtrees but not the other global variables (that hold configuration options), since the latter are only accessed in the main thread so those do not share a race condition
• calling AXL_Config on a transfer that has already been dispatch is a race condition (since there is no lock on the individual file_list kvtrees) and not supported
• calling AXL_Config for the DEBUG settings is race condition (since axl_debug is not protected but is accessed from within the worker threads) while there are already dispatched transfers and will not be fixed

[tonyhutter]

@rhaas80 could you open a PR with those changes?

[rhaas80]

@rhaas80 could you open a PR with those changes?

Well it would be a pull request onto my own rhaas/keyvalue branch which itself still needs to be approved. So I can make it a work-in-progress. An actual pull does not make much sense.

[rhaas80]

Here's a pull request, but as said it does not make that much sense to pull into my own branch. https://github.com/rhaas80/AXL/pull/1