thread safety?

[marler8997]

We ran into some intermittent issues using this library in a multi-threaded application. It looks like there is some "thread unsafe" behavior such as

• error_table and current in src/error.c
• path in get_efivarfs_path in src/efivarfs.c

The question is whether this library should be made to be thread-safe and whether contributions to make it so would be accepted. If we submitted contributions, what techniques would be acceptable (thread-locals, mutex locking, remove static/global caching etc). Also if implemented, should thread-safety be an option during configuration?

[vathpela]

If we've got something that's not thread safe, it's a straight up bug, and we need to fix it. I suspect that means thread locals in all cases, but if there's a place that won't work, I'm open to other suggestions.

The harder question: how do we efficiently test for what we might have wrong in this regard?

[marler8997]

The harder question: how do we efficiently test for what we might have wrong in this regard?

I think we were able to reproduce the threading errors fairly reliably by creating a small multi-threaded test program. Let me see if I can find the code and post it back here.

[marler8997]

Ok here's a script/program that will reliably reproduce the error:

#!/usr/bin/env sh
set -ex
gcc efivar-multithreaded-test.c -lpthread \
    $(pkg-config efivar --cflags --libs)

rm -rf scratch
mkdir scratch

set +x
echo "================================================================================"
echo "testing 1 thread..."
echo "================================================================================"
set -x
LIBEFIVAR_OPS=efivarfs EFIVARFS_PATH=$(realpath scratch)/ ./a.out 1

set +x
echo "================================================================================"
echo "1 thread worked, testing 2 threads..."
echo "================================================================================"
set -x
LIBEFIVAR_OPS=efivarfs EFIVARFS_PATH=$(realpath scratch)/ ./a.out 2

#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <alloca.h>
#include <pthread.h>

// returns: the number of threads created
static int multi_pthread_create(pthread_t *threads, unsigned count, void *(*start_routine)(void *))
{
  unsigned i = 0;
  for (; i < count; i++) {
    int result = pthread_create(&threads[i], NULL, start_routine, NULL);
    if (result != 0) {
      perror("pthread_create");
      break;
    }
  }
  return i;
}

#define TEST_SUCCESS NULL
#define TEST_FAIL ((void*)1)
static const char *test_result_str(void *result)
{
  return (result == TEST_SUCCESS) ? "SUCCESS" : "FAIL";
}

// returns: 0 on success
static int multi_pthread_join(pthread_t *threads, unsigned count, void **worst_result)
{
  for (unsigned i = 0; i < count; i++) {
    void *result;
    int join_result = pthread_join(threads[i], &result);
    if (join_result != 0) {
      perror("pthread_join");
      return 1;
    }
    printf("[MAIN-THREAD] child %d exited with %s\n", i, test_result_str(result));
    if (result != NULL) {
      *worst_result = result;
    }
  }
  return 0;
}

#include <efivar.h>
#define LOOP_COUNT 100

static void *loop_get_variable_size_test(void *_)
{
  //printf("[DEBUG] test running on new thread!\n");
  for (unsigned i = 0; i < LOOP_COUNT; i++) {
    size_t size;
    efi_guid_t guid = {0};
    int result = efi_get_variable_size(guid, "foo2", &size);
    if (result == 0 || errno != ENOENT) {
      printf("fail, iteration=%u, result=%d errno=%d, expected error\n", i, result, errno);
      return TEST_FAIL;
    } else {
      //printf("[DEBUG] iteration=%u, result=%d errno=%d\n", i, result, errno);
    }
  }
  return TEST_SUCCESS;
}

static int multithreaded_test(size_t count, void *(*test_func)(void *))
{
  pthread_t *threads = alloca(sizeof(pthread_t) * count);
  if (count != multi_pthread_create(threads, count, test_func)) {
    // error already logged
    return 1;
  }
  void *worst_result = TEST_SUCCESS;
  if (multi_pthread_join(threads, count, &worst_result)) {
    // error already logged
    return 1;
  }
  printf("worst result %s\n", test_result_str(worst_result));
  return (worst_result == TEST_SUCCESS) ? 0 : -1;
}

int main(int argc, char *argv[])
{
  if (argc != 2) {
    printf("Usage: %s THREAD_COUNT\n", argv[0]);
    return 1;
  }
  int thread_count = atoi(argv[1]);
  printf("thread count %d\n", thread_count);
  return multithreaded_test(thread_count, loop_get_variable_size_test);
}

[marler8997]

fixed here: cff88dd