Thread safety of UTC/TAI conversions

[bluescarni]

Hello!

Although in #27 it is stated that erfa is thread-safe, I think that UTC/TAI conversions currently are not, due to the way leap second tables are managed.

In order to support access to and modification of leap seconds information at runtime, two global variables called changes and NDAT (defined in erfadatextra.c) are used. The problem is that access to these variables is not protected by mutexes/critical sections/atomics and thus it is subject to data races in multithreaded contexts.

In particular, even if the user never sets up custom leap second tables, on the first execution of eraUtctai(), the eraDatini() function will be invoked (via eraDat()) to initialise the changes and NDAT variables with the default builtin leap seconds tables. This means that it is not safe to invoke eraUtctai() concurrently from multiple threads as the threads would be running the same initialisation code at the same time.

I first noticed this issue on a project of mine, where I started experiencing erratic segfaults in the CI pipeline after I started using erfa for UTC/TAI conversions. After many tries, running the code through the address sanitizer finally showed a null pointer dereference in eraDatini().

To be clear, I understand that it would be difficult to add thread safety to the eraGetLeapSeconds()/eraSetLeapSeconds() functions - this may have a performance impact and it may require either switching to recent C versions and/or platform-specific complications.

However, I think it should be possible to make at least the use of eraUtctai() thread-safe, with the caveat that modifying the leap seconds tables needs to be done from a single thread while no other code is accessing the tables from other threads.

What I am proposing is to avoid invoking eraDatini() from eraDat(), and instead initialise changes and NDAT directly at program startup with the builtin leap seconds tables. E.g., something along these lines (in erfadatextra.c):

// The builtin leap seconds table.
static const eraLEAPSECOND builtin_changes = { /* builtin leap seconds info here */};

// The leap seconds table that can be altered at runtime.
static eraLEAPSECOND *changes = (eraLEAPSECOND *)builtin_changes;
static int NDAT = sizeof(builtin_changes) / sizeof(builtin_changes[0]);

Would that sound reasonable?

[mhvk]

@bluescarni - hmm, that is indeed a problem. I like the idea to make sure that at least things work always correctly if one never updates the leap-second table at all.

I can see how it might help to switch things around so that no initialization is needed for the default built-in case, and a PR to do that would be most welcome! One thing to keep in mind is that the present code tried hard to minimize changes to dat.c - this so that if the upstream SOFA library is updated, we have minimal work to do to adjust it. I think your suggestion would imply moving the built-in leap-second table from dat.c to another place. Probably, that is fine, we just have to add a comment in dat.c to make clear for future changes what exactly needs to be done.