FIX(plugins): Use atomic operations in Link plugin

[autumnontape]

The Link plugin uses an inherently racy protocol to communicate with applications: shared memory with no means of synchronization. This commit makes no changes to this protocol and shouldn't break anything that was working before. That means it doesn't eliminate race conditions, but it does allow the plugin to work without the risk of data races specifically, which are undefined behavior.

This is done by using 32-bit atomic operations to copy the LinkedMem struct out of shared memory before reading from it. This should work well, since every field of LinkedMem has a size that's a multiple of 32 bits, and 32-bit atomics are widely supported.

With this change, applications can avoid data races with the Link plugin by also accessing shared memory exclusively with 32-bit atomic operations. Applications that already work with the plugin should continue to work.

---

This is marked as a "fix," but as far as I know, the bugs it fixes are only hypothetical right now, so I'm offering it for your consideration.

Checks

• My commits follow the commit guidelines

[Krzmbrzl]

(Be assured that reviewing this is on my TODO list - the PR has not been forgotten. I just don't have the time to do it atm)

[Krzmbrzl]

I'm wondering whether we have a way to ensure that each individual member in LinkedMem is <= uint32 and is aligned to 32-bit integers as this seems to be required in order for this all to work out

[autumnontape]

The individual fields don't necessarily need to have any particular size or alignment for this to work, though if they're larger than 4 bytes (as the array fields are) or not aligned to 4 bytes, the plugin might see some bytes from before a shared write and some bytes from after it within the same field.

That might cause undefined behavior if certain bit representations don't correspond to valid values for any of the fields in LinkedMem in the C++ implementation in use. I also think it is important that there's no padding in LinkedMem, since that could cause undefined behavior in SharedMemory::write, assuming memcpy works the same as C++20's bit_cast when used for type punning like this. LinkedMem probably isn't going to change often if ever, so I think a comment saying to be careful not to introduce padding to it should be enough.

I hadn't considered the possibility that atomic<uint32_t> might not have the same size as uint32_t, or that it might not be lock-free. I think the only way to check the latter at compile time in C++14 is using ATOMIC_{INT,LONG}_LOCK_FREE. I also assumed the result of mmap/MapViewOfFile would be suitably aligned for 32-bit atomics. This should always be true for mmap, but I surprisingly can't find explicit confirmation that MapViewOfFile returns a pointer with any particular alignment.

For common desktop platforms (and mobile platforms as well), none of these C++ implementation-based edge cases should happen. I can find examples of code in prominent projects that assumes the pointer returned by MapViewOfFile has a particular alignment, for example this code in Firefox that assumes it's sufficiently aligned for a struct containing int64_t.

I'll make the changes I think make sense to address this stuff and push them so you can see what you think.

[autumnontape]

Sorry for all the force pushes.

[Hartmnt]

Sorry for all the force pushes.

Force pushes are the correct way to do this.

[Krzmbrzl]

I also think it is important that there's no padding in LinkedMem, since that could cause undefined behavior in SharedMemory::write, assuming memcpy works the same as C++20's bit_cast when used for type punning like this.

Are you referring to this quote from the docs?

The values of padding bits in the returned To object are unspecified.

if so, then this would not be a problem for us as we don't care about the values of padding bits anyway.
The only issue that I can see with padding bits is that both ends of the Link interface (aka: the Link plugin and whatever wants to connect to it) used different compilers that for some reason chose to use different padding for the struct. I'm not sure if this is even legal for compilers to do, but if they do, then we have a problem. But in that case the Link plugin doesn't work at all, so probably not really related to this PR...

[autumnontape]

I also think it is important that there's no padding in LinkedMem, since that could cause undefined behavior in SharedMemory::write, assuming memcpy works the same as C++20's bit_cast when used for type punning like this.

Are you referring to this quote from the docs?

The values of padding bits in the returned To object are unspecified.

if so, then this would not be a problem for us as we don't care about the values of padding bits anyway.

I'm referring to this part:

A bit in the value representation of the result is indeterminate if it

• does not correspond to a bit in the value representation of From (i.e. it corresponds to a padding bit), or
• [...]

For each bit in the value representation of the result that is indeterminate, the smallest object containing that bit has an indeterminate value; the behavior is undefined unless that object is of unsigned char or std::byte type.

It sounds like making an int out of padding bits with bit_cast is immediate UB.

[Krzmbrzl]

It sounds like making an int out of padding bits with bit_cast is immediate UB.

To me read reads as merely yielding an indeterminate value - aka: it is guaranteed to yield some random int, but we can't know which (even if we know the source bit pattern it is copied from). But for us this doesn't matter as all possible 4 byte bit patterns are valid unsigned 32-bit integer representations and therefore, we shouldn't have to worry about what exactly we are copying (and since padding bits will only end up as padding bits again, we indeed don't care about their values)

[autumnontape]

It says behavior is undefined if the smallest object containing any indeterminate bit isn't unsigned char or std::byte. Since an int isn't made up of unsigned char, I think we'd be in trouble.

[Krzmbrzl]

Oh yeah indeed. On the other hand though, I can't imagine that we have this kind of issue with memcpy as long as the target type does not have any invalid bit representations. But better safe than sorry, I guess :D

[autumnontape]

Yeah, it's really hard to say without a copy of the C++14 standard. I also don't know what happens when you write an indeterminate value to an atomic. I changed the comment to say "possible undefined behavior." Any other comments or changes to suggest?

[Krzmbrzl]

LGTM - let's see how it'll work out in practice :)

[Krzmbrzl]

Thanks for your contribution and your patience :D

[Krzmbrzl]

💚 All backports created successfully

Status	Branch	Result
✅	1.5.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details