How about thread safety?

[IdWV]

When looking through the code I have the impression that the code should be thread safe, as long as you only access the uc_vm_t object from the thread where the uc_vm_execute() function is running.

With one exception, all uc_resource_type_t *objects in the modules in the /lib directory are global. I think this means that running 2 scripts using the same module in different threads can give undesired results.

To solve this, I think a module should be able to store a pointer in uc_vm_t (together with a cleanup function), and request that back. Maybe by using that cleanup function pointer as key.

I couldn't find it, is such a storage implemented?

[jow-]

Yes, this is indeed a limitation in the current code. The fix is rather straightforward though. The resource types are already stored per vm context, but due to lazyness the result pointer got stored in a global variable for easy access.

Affected modules can be refactored like this:

diff --git a/lib/fs.c b/lib/fs.c
index e3f8a4a..23c5495 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -71,7 +71,6 @@
 #define err_return(err) do { last_error = err; return NULL; } while(0)

 //static const uc_ops *ops;
-static uc_resource_type_t *file_type, *proc_type, *dir_type;

 static int last_error = 0;

@@ -511,7 +510,7 @@ uc_fs_popen(uc_vm_t *vm, size_t nargs)
    if (!fp)
        err_return(errno);

-   return uc_resource_new(proc_type, fp);
+   return uc_resource_new(ucv_resource_type_lookup(vm, "fs.proc"), fp);
 }

@@ -1178,7 +1177,7 @@ uc_fs_open(uc_vm_t *vm, size_t nargs)
        err_return(i);
    }

-   return uc_resource_new(file_type, fp);
+   return uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), fp);
 }

 /**
@@ -1237,7 +1236,7 @@ uc_fs_fdopen(uc_vm_t *vm, size_t nargs)
    if (!fp)
        err_return(errno);

-   return uc_resource_new(file_type, fp);
+   return uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), fp);
 }

@@ -1438,7 +1437,7 @@ uc_fs_opendir(uc_vm_t *vm, size_t nargs)
    if (!dp)
        err_return(errno);

-   return uc_resource_new(dir_type, dp);
+   return uc_resource_new(ucv_resource_type_lookup(vm, "fs.dir"), dp);
 }

 /**
@@ -2400,7 +2399,7 @@ uc_fs_mkstemp(uc_vm_t *vm, size_t nargs)
        err_return(errno);
    }

-   return uc_resource_new(file_type, fp);
+   return uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), fp);
 }

 /**
@@ -2770,8 +2769,8 @@ uc_fs_pipe(uc_vm_t *vm, size_t nargs)

    rv = ucv_array_new_length(vm, 2);

-   ucv_array_push(rv, uc_resource_new(file_type, rfp));
-   ucv_array_push(rv, uc_resource_new(file_type, wfp));
+   ucv_array_push(rv, uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), rfp));
+   ucv_array_push(rv, uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), wfp));

    return rv;
 }
@@ -2873,11 +2872,11 @@ void uc_module_init(uc_vm_t *vm, uc_value_t *scope)
 {
    uc_function_list_register(scope, global_fns);

-   proc_type = uc_type_declare(vm, "fs.proc", proc_fns, close_proc);
-   file_type = uc_type_declare(vm, "fs.file", file_fns, close_file);
-   dir_type = uc_type_declare(vm, "fs.dir", dir_fns, close_dir);
+   uc_type_declare(vm, "fs.proc", proc_fns, close_proc);
+   uc_type_declare(vm, "fs.file", file_fns, close_file);
+   uc_type_declare(vm, "fs.dir", dir_fns, close_dir);

-   ucv_object_add(scope, "stdin", uc_resource_new(file_type, stdin));
-   ucv_object_add(scope, "stdout", uc_resource_new(file_type, stdout));
-   ucv_object_add(scope, "stderr", uc_resource_new(file_type, stderr));
+   ucv_object_add(scope, "stdin", uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), stdin));
+   ucv_object_add(scope, "stdout", uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), stdout));
+   ucv_object_add(scope, "stderr", uc_resource_new(ucv_resource_type_lookup(vm, "fs.file"), stderr));
 }

Along such a refactoring, it probably makes sense to introduce a new instantiation function for resource values as well, something along the lines of

static uc_value_t *
ucv_resource_create(uc_vm_t *vm, const char *typename, void *value)
{
    uc_resource_type_t *t = NULL;

    if (typename && (t = ucv_resource_type_lookup(vm, typename)) == NULL)
        return NULL;

    return uc_resource_new(t, value);
}

Which would turn the refactoring into

diff --git a/lib/fs.c b/lib/fs.c
index e3f8a4a..e7dc52f 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -71,7 +71,6 @@
 #define err_return(err) do { last_error = err; return NULL; } while(0)

 //static const uc_ops *ops;
-static uc_resource_type_t *file_type, *proc_type, *dir_type;

 static int last_error = 0;

@@ -511,7 +510,7 @@ uc_fs_popen(uc_vm_t *vm, size_t nargs)
    if (!fp)
        err_return(errno);

-   return uc_resource_new(proc_type, fp);
+   return ucv_resource_create(vm, "fs.proc", fp);
 }

@@ -1178,7 +1177,7 @@ uc_fs_open(uc_vm_t *vm, size_t nargs)
        err_return(i);
    }

-   return uc_resource_new(file_type, fp);
+   return ucv_resource_create(vm, "fs.file", fp);
 }

 /**
@@ -1237,7 +1236,7 @@ uc_fs_fdopen(uc_vm_t *vm, size_t nargs)
    if (!fp)
        err_return(errno);

-   return uc_resource_new(file_type, fp);
+   return ucv_resource_create(vm, "fs.file", fp);
 }

@@ -1438,7 +1437,7 @@ uc_fs_opendir(uc_vm_t *vm, size_t nargs)
    if (!dp)
        err_return(errno);

-   return uc_resource_new(dir_type, dp);
+   return ucv_resource_create(vm, "fs.dir", dp);
 }

 /**
@@ -2400,7 +2399,7 @@ uc_fs_mkstemp(uc_vm_t *vm, size_t nargs)
        err_return(errno);
    }

-   return uc_resource_new(file_type, fp);
+   return ucv_resource_create(vm, "fs.file", fp);
 }

 /**
@@ -2770,8 +2769,8 @@ uc_fs_pipe(uc_vm_t *vm, size_t nargs)

    rv = ucv_array_new_length(vm, 2);

-   ucv_array_push(rv, uc_resource_new(file_type, rfp));
-   ucv_array_push(rv, uc_resource_new(file_type, wfp));
+   ucv_array_push(rv, ucv_resource_create(vm, "fs.file", rfp));
+   ucv_array_push(rv, ucv_resource_create(vm, "fs.file", wfp));

    return rv;
 }
@@ -2873,11 +2872,11 @@ void uc_module_init(uc_vm_t *vm, uc_value_t *scope)
 {
    uc_function_list_register(scope, global_fns);

-   proc_type = uc_type_declare(vm, "fs.proc", proc_fns, close_proc);
-   file_type = uc_type_declare(vm, "fs.file", file_fns, close_file);
-   dir_type = uc_type_declare(vm, "fs.dir", dir_fns, close_dir);
+   uc_type_declare(vm, "fs.proc", proc_fns, close_proc);
+   uc_type_declare(vm, "fs.file", file_fns, close_file);
+   uc_type_declare(vm, "fs.dir", dir_fns, close_dir);

-   ucv_object_add(scope, "stdin", uc_resource_new(file_type, stdin));
-   ucv_object_add(scope, "stdout", uc_resource_new(file_type, stdout));
-   ucv_object_add(scope, "stderr", uc_resource_new(file_type, stderr));
+   ucv_object_add(scope, "stdin", ucv_resource_create(vm, "fs.file", stdin));
+   ucv_object_add(scope, "stdout", ucv_resource_create(vm, "fs.file", stdout));
+   ucv_object_add(scope, "stderr", ucv_resource_create(vm, "fs.file", stderr));
 }

Would you be willing to spin a PR applying this refactoring to the various lib/*.c modules?

[IdWV]

Well, I can try. I'm a dinosaur which learned programming in K&R C. But never used git(hub), so 'spinning' a pull request is new to me.

[jow-]

I see :) I'll see if I can do the changes tonight then. In the meanwhile I would be good if you could manually apply the changes outlined above and confirm whether they indeed solve your thread safety issues.

[IdWV]

I can confirm that this patch solves the module reuse problem for the fs module. Multithreading also works, except for the uc_fs_error()/err_return() function. For the fs.file and fs.dir resource that could be solved by not storing the FILE or DIR pointer in the uc_resource_t, but a { FILE *fp; int last_error; } struct pointer. Looking through the lib dir I see almost every module has a static last_error value/struct. Maybe it's an idea to use the _Thread_local keyword? If it works it is an easy fix. But I don't know if it pulls libpthread.so into a singlethreaded program. And of course it's ugly to bind a variable to a thread instead of the vm object.

[IdWV]

Further searching learned that the vm_registry seems to be designed for this. Is there somewhere documentation available about all these API functions?

[sebastianertz]

There are so many global variables, it will be a lot of work to remove them.

This is what I found quickly:

// lib/fs.c
static uc_resource_type_t *file_type, *proc_type, *dir_type;
static int last_error = 0;

// lib/log.c
static char log_ident[32];

// lib/math.c
static bool srand_called = false;

// lib/nl80211.c
static struct {
    int code;
    char *msg;
} last_error;

static uc_resource_type_t *listener_type;
static uc_value_t *listener_registry;
static uc_vm_t *listener_vm;

// lib/resolv.c
static struct {
    int code;
    char *msg;
} last_error;

// lib/rtnl.c
static struct {
    int code;
    char *msg;
} last_error;
static uc_resource_type_t *listener_type;
static uc_value_t *listener_registry;
static uc_vm_t *listener_vm;

// lib/socket.c
static struct {
    int code;
    char *msg;
} last_error;

// lib/ubus.c
static struct {
    enum ubus_msg_status code;
    char *msg;
} last_error;
static uc_resource_type_t *subscriber_type;
static uc_resource_type_t *listener_type;
static uc_resource_type_t *request_type;
static uc_resource_type_t *notify_type;
static uc_resource_type_t *object_type;
static uc_resource_type_t *defer_type;
static uc_resource_type_t *conn_type;
static uint64_t n_cb_active;
static bool have_own_uloop;
static struct blob_buf buf;

// lib/uci.c
static int last_error = 0;
static uc_resource_type_t *cursor_type;

// lib/uloop.c
static uc_resource_type_t *timer_type, *handle_type, *process_type, *task_type, *pipe_type;
static uc_resource_type_t *interval_type;
static uc_resource_type_t *signal_type;
static uc_value_t *object_registry;
static int last_error = 0;

// types.c
uc_list_t uc_object_iterators = {
    .prev = &uc_object_iterators,
    .next = &uc_object_iterators
};

// vm.c
static uc_value_t *exception_prototype = NULL;
static uc_resource_type_t uc_vm_object_iterator_type = {
    .name = "object iterator",
    .free = uc_vm_object_iterator_free
};

[IdWV]

True. But it's not hard to do, and for my purposes the OpenWrt only modules are not important. That removes about 80% of the code to be adjusted, when I'm not mistaken.

But that 'uc_list_t uc_object_iterators' thing is a nasty one. It is used in a static object delete function (static ucv_free_object_entry(), in types.c), called from an external library json-c, without possibility to provide a uc_vm_t pointer.