#388 memcpy_s removal

[RyadaProductions]

Please check if the PR fulfills these requirements

• [x] The commit message follows our guidelines
• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Documentation has been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Changes the memcpy_s instances to use memcpy directly but adds asserts for debug time issues and if applicable nullptr checks on the destination/source

Is there an open issue that this resolves? If so, please link it here.

What is the current behavior? (You can also link to an open issue here) N/A

What is the new behavior (if this is a feature change)? N/A

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) No breaking change for the end user

Other information: N/A

[tannergooding]

Changes the memcpy_s instances to use memcpy directly but adds asserts for debug time issues and if applicable nullptr checks on the destination/source

Why? What's the reasoning for not using an official C/C++ API in favor of rolling our own equivalent?

[RubyNova]

Why? What's the reasoning for not using an official C/C++ API in favor of rolling our own equivalent?

Because we can't actually use it in all locations and the errno_t stuff is additional faff I can't be arsed with, to put it flatly. It's easier for us to just validate the memcpy call is correct and be done. Only windows supports memcpy_s and I'd rather not maintain two branches of the code in this case.

[RyadaProductions]

On the case of reusing a selfmade method for the assertions: Technically I could add a method that we can reuse like memcpy_validation but since we don't use memcpy that often I figured asserting it at the start of a method where we expect certain things to be passed in is cleaner. Ofc if we want a single method that does the assertion as well as the memcpy I'm not opposed to that but in this case I feel this is safer.

[tannergooding]

Ofc if we want a single method that does the assertion as well as the memcpy I'm not opposed to that but in this case I feel this is safer.

Rolling your own validation logic, especially logic that has to be duplicated at each callsite, is almost never safer ;)

Centralizing the checks to one place such that things can be correctly accounted for and will always be accounted for is the safest thing you can do.

In this case, if for some reason memcpy_s isn't available (such as GCC/Clang decided to not implement Annex K) then it'd be better to simply define a memcpy_s shim with the right surface area and to place the validation logic in that.

If for some reason you don't want to handle errno_t at each call site, then likewise define some C++ MemoryCopy utility that wraps memcpy_s, checks errno_t, and throws a proper exception on failure.

[RubyNova]

Yeah they explicitly did not implement it in Linux/macOS/console land because "its a stupid API" from what I remember of reading up on it previously.

Very helpful, I know. I'm not in pain at all. :)

[tannergooding]

So why not define our own MemoryCopy that wraps the right APIs where available and does the checks otherwise.

This centralizes everything, allows easy modification in the future if we need (say we want to disable the checks in release builds), allows ensuring consistent behavior (such as throwing C++ exceptions for failures or asserting correct results), etc.

[RubyNova]

That is a possible solution. The original GH issue was just the minimally required work and I kind of wrote it out of frustration mid-Vulkan pipeline fixing ages ago. That's probably why. :D

[tannergooding]

Ah, I missed that there was an issue at all.

Would be helpful to have it show as a regular issue https://github.com/novelrt/NovelRT/issues/388 and not as link it here

I think it would be much better/safer/extensible to make our own wrapper method in such cases. Having manually placed validation at each memcpy callsite is overall error prone and easy to forget to do. Wrapper ensures it always happens, can do the relevant kind of checks, expose the right kind of errors, and can easily change behavior for all callsites based on perf, security, or other needs.

[FiniteReality]

Frankly, we should update memcpy calls to std::copy over a std::span/gsl::span. In addition, the APIs that return raw pointers should be updated to also use a span type - the C API would translate this to something along the lines of void* fn(size_t* size) and write the span's size to the size parameter if it's non-null, as is typical for C APIs.