Fix memcpy in byte_slice constructor

[vtnerd]

There's technically an overflow in byte_slice.cpp that I caught while simply scanning the code. This can occur if the total memory being copied exceeds size_t. This is unlikely to ever trigger, because the platform probably needs sizeof(size_t) == 4 && sizeof(void*) == 8 (or similar) to occur. And somehow a malloc to copy all of this data needs to succeed too. Therefore, this is primarily just defensive programming to be strictly standards compliant.

[vtnerd]

I wasn't up-to-date with master, so the last force push was a rebase.

[iamamyth]

This can occur if the total memory being copied exceeds size_t

If this happens, do you really want a silent failure? It'd mean the caller tried to produce a copy exceeding the whole of memory, very likely a bug, and probably unrecoverable.

[vtnerd]

The next line catches the overflow. remove_prefix returns the number of bytes actually removed, which is less than requested in the overflow case.

[iamamyth]

If you want to be paranoid about overflows, it would seem you need to check here for MAX wrapping back to 0:
https://github.com/monero-project/monero/pull/9583/files#diff-ae5d1c840afa6e07bbc29bc1432800ab4ef3bb779067e5e45b0a4943a6e65412L155

But if you include this check, based on my reading of allocate_slice and span, out.remove_prefix will never fail, so both the min and remove_prefix checks guard against the impossible.

[vtnerd]

The idea was to catch the overflow later on the remove_prefix line. It gets caught either way.

[iamamyth]

It gets caught either way.

If it wraps to exactly zero, it's not caught, because the whole block is gated by if (space_needed).

[vtnerd]

Yup, it has defined behavior in that situation, but it is still incorrect.

[vtnerd]

Force pushed a different change.