Fix some sources of undefined behavior

[slipher]

• Fix unsafely populating vector of unknown size via memset
• Cut down on memset and memcpy generally. The problem with them is the undefined behavior with null pointers
• Rewrite Color to avoid UB array indexing style

[illwieckz]

One thing I like with memcpy is that compilers usually know the best way to copy and use intrinsics as much as possible, usually even in debug mode, while you need some levels of compiler optimizations to get MatrixCopy vectorized by the compiler.

I wonder if we can have a custom memcpy with an assert it's not null (for non-user provided data), and one with a check otherwise, when std::copy_n is not available.

[illwieckz]

That UB problem of memcpy is very annoying because memcpy is very efficient and is assumed to do the best for the task…

[slipher]

Well, MatrixCopy still does memcpy :) This case is not one of the risky ones as the array is fixed-length. Here I'm really just trying to reduce the number of times memcpy appears in the source.

Also std::copy and friends are expected to shell out to memcpy when applicable (nonzero length, struct is POD or whatever)

[illwieckz]

Well, MatrixCopy still does memcpy :)

Ahah, yes, that's true for this one, I'm even the one who did it: d8089d6 😄️

[illwieckz]

This one may be a good candidate for a custom memcpy with a check if there is no alternative to memcpy.

[slipher]

Right. In this case the types are incompatible according to C++ rules, but we know the layout is the same.

[illwieckz]

Can memcpy_s be of any use for us?

Quoting https://en.cppreference.com/w/c/string/byte/memcpy :

errno_t memcpy_s( void *restrict dest, rsize_t destsz, const void *restrict src, rsize_t count ); (2)

2. Same as (1), except that the following errors are detected at runtime and cause the entire destination range [dest, dest+destsz) to be zeroed out (if both dest and destsz are valid), as well as call the currently installed constraint handler function:

• dest or src is a null pointer
• destsz or count is greater than RSIZE_MAX
• count is greater than destsz (buffer overflow would occur)
• the source and the destination objects overlap

The behavior is undefined if the size of the character array pointed to by dest < count <= destsz; in other words, an erroneous value of destsz does not expose the impending buffer overflow.

As with all bounds-checked functions, memcpy_s only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including <string.h>.

[slipher]

Can memcpy_s be of any use for us?

I don't think so. The advantage would be that calling it with a null pointer is now a defined behavior: it definitely crashes. But this is outweighed by the negatives. Calculating the destination writable size seems like a pain in the ass and could introduce new crashes because we did it wrong. Also I imagine that compilers would not be able to inline this, so we would lose efficiency.