Workaround strncpy() in OpenArena QVMs

[zturtleman]

Changes in the macOS 10.9 SDK and glibc 2.37 broke overlapping dest and src
in strncpy() used by OpenArena QVMs. (Technically it was undefined behavior.)

This PR changes the QVM strncpy() syscall to not alter the dest if dest and src are equal. This fixes weapon barrel models for OpenArena 0.8.8.

---

Arguably it should fill the end of the dest buffer past the NUL-terminator with 0 bytes as per strncpy() definition. However nothing in Quake 3 is likely to depend on this and this case is undefined behavior.

This replaces my previous over-engineered PR #659.

[timangus]

Thoughts:

1. This is only catching the case where dest and src are the same; if dest = src + 1 that's still undefined, and this change will still let that through. I'm not saying we should do anything about it, just pointing it out.
2. I'm a bit reticent about introducing workarounds for bugs that could easily just be fixed at source, though I understand the reasons when it comes to OpenArena 🙄. Workarounds like this have a nasty habit of causing "bugs" in otherwise "working" code that was relying on undefined behaviour. Having said that...
3. ...the semantic expectation of doing a str(n)cpy with identical dest and src would be a no-op, so maybe it's fine.

Perhaps add an else { Com_DPrintf(...) so that a developer has some indication they're doing something wrong?

[zturtleman]

On further thought, yes it should clear the rest of the dest buffer after the NUL-terminator to follow the specification for strncpy(). I think this is the main way this workaround could end up negatively affect things. I'll update it soon.

(Someday I'll stop making PRs the same day I write the code.)

Thoughts:

I agree with your points.

2. I'm a bit reticent about introducing workarounds for bugs that could easily just be fixed at source, though I understand the reasons when it comes to OpenArena 🙄.

The strncpy() that ioquake3 provides to QVMs changed and caused the problem. glibc and macOS changed it with proper versioning so existing applications are not affected.

ioquake3 is being compiled for newer glibc and macOS SDK versions and has to adapt. Unfortunately there is no one to pass the blame on to in my opinion. QVMs should not have to be updated for platform changes.

It has been fixed in OpenArena QVMs but I don't know the timeline for a new release. I think it's reasonable to expect older versions and mods to work on ioquake3 targeting newer glibc.

Perhaps add an else { Com_DPrintf(...) so that a developer has some indication they're doing something wrong?

I think it would be appropriate to add a Com_Error() to Q_strncpyz() to catch overlapping dest and src in VMs during development.

I don't think the engine needs to warn that a QVM strncpy() call use to work and the engine is making it continue to work but the code could be different to meet current expectations.

[timangus]

QVMs should not have to be updated for platform changes.
I think it's reasonable to expect older versions and mods to work on ioquake3 targeting newer glibc.
I don't think the engine needs to warn that a QVM strncpy() call use to work and the engine is making it continue to work but the code could be different to meet current expectations.

I think it's arguable both ways, honestly; there's no obvious answer. On the one hand you could say that the syscall strncpys should allow overlaps since in the past they did and we can't change the semantics of syscalls, even if they're used incorrectly. On the other hand you could say there's a reasonable expectation that if you use a strncpy like thing, you should expect strncpy like behaviour, including crashes/asserts if you do the wrong thing. But to be clear I'm not saying we should allow old mods to not work or crash ioq3.

My thinking with adding a DPrintf is that it informs the developer that their code is wrong and encourages them to correct it, even if it isn't technically wrong since the historical Q3 syscall interface strncpy technically allows overlaps. A similar, but more egregious example that springs to mind is 50a10f2 (58c8175). If you don't inform about these kinds of things then developers simply won't know that their code is potentially buggy. That being said I don't feel that strongly about it, in particular because copying a string to itself is not an obviously wrong thing to do semantically, even if it has always been technically wrong.

I do think things get more murky when you're calling strncpy with dest and src that are different but overlapping though. I'm not sure what we should do in that situation. Hopefully there are no examples of this actually happening and we can safely ignore it 😅.

I think it would be appropriate to add a Com_Error() to Q_strncpyz() to catch overlapping dest and src in VMs during development.

Sounds good.

[zturtleman]

Sorry, I'm abandoning addressing this issue due to changes in my software priorities. Someone else is free to pick up addressing this / #639.