Attempt to fix the identified thread safety bugs #155
Conversation
| @@ -28,18 +29,36 @@ | |||
| #endif | |||
|
|
|||
| static char const default_efivarfs_path[] = "/sys/firmware/efi/efivars/"; | |||
| static char *efivarfs_path; | |||
|
|
|||
| static char const * | |||
| get_efivarfs_path(void) | |||
There was a problem hiding this comment.
It looks like this purpose of this function was to support lazy initialization, but if you're going to use a CONSTRUCTOR to initialize efivarfs_path, then this function seems unnecessary. Its contents could be put into the CONSTRUCTOR function instead. This removes need to check whether efivarfs_path is NULL both in the constructor and any time it is being used.
There was a problem hiding this comment.
I'm not sure it does actually obviate that—though admittedly the situation where it doesn't is fraught with bad ideas. Consider a program which links against libefivar.so and another library, libmistakes.so. libmistakes.so does not link against libefivar.so or have any DT_NEEDED object references to it, but has a DT_INIT constructor that does something like:
static void CONSTRUCTOR init_mistakes(void)
{
int (*gnvn)(efi_guid_t **guid, char **name);
char *name;
efi_guid_t *guid;
int rc;
gnvn = dlsym(RTLD_DEFAULT, "efi_get_next_variable_name");
if (!gnvn)
return;
rc = gnvn(&guid, &name);
if (rc >= 0)
printf("first EFI variable is %s\n", name);
}
Since libmistakes.so has no DT_NEEDED pointing at libefivar.so, the DT_INIT sorting is not required to ensure that init_mistakes() is called after init_efivarfs_path(); they could be called in any order. Though that does mean the error check on the allocation also needs to be in get_efivarfs_path(), rather than init_efivarfs_path(). I've fixed that in 3d19e7d.
I realize this sounds completely contrived but the basic outline here is more or less how kdeinit works...
Anyway, that aside, are you okay with what I've done in 77f77a4c6d6 ?
vathpela
force-pushed
the
thread-safety
branch
3 times, most recently
from
1e7e321
to
77f77a4
Compare
Because the error log array can be changed in any call, it could be modified by more than one thread at once. This means that the results won't make any sense - even absent actual execution errors as a result, the best case is intermixed error logs from different threads. This makes the error log pointer thread local, so that each thread will effectively get its own error log. Fixes github issue rhboot#154 Signed-off-by: Peter Jones <pjones@redhat.com>
To avoid having problems when the environment changes (due to setenv() or whatever), ensure that there's an initial call to get_efivarfs_path(), and that it duplicates the result into a freshly allocated string. Fixes github issue rhboot#154 Signed-off-by: Peter Jones <pjones@redhat.com>
This adds a test case that just spawns a bunch of threads quickly and calls efi_get_variable() on an empty directory. It fails pretty reliably without either of the following commits: 65a3ad0 error.c: make our 'global' state be thread local 3d19e7d efivarfs.c: Call get_efivarfs_path() from a constructor Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
force-pushed
the
thread-safety
branch
from
77f77a4
to
73fae6c
Compare
|
(I'm going to assume that's a yes, if you're not okay with that I can always revert it later and do something else.) |
|
What you had was fine, it may or may not be able to be improved with my suggestion. Thanks for taking the time to bring in the new test as well. |
In #154 , @marler8997 noticed we have some thread safety bugs (though I think one of them was actually a getenv() usage bug). This PR attempts to resolve those that were identified.
I don't have a particularly good way to test this, nor to actually detect thread safety bugs, so any help would be handy.