Fix a potential overflows on cpywithpad function.

[araujo]

Summary

Brooks Davis has pointed out a possible overflow on this function
while ago. The Nvme standard calls for '' rather than '\0' padding,
the prop memcpy+memset does the job right, but contains potential for
overflow.

Test Plan

Smoke test on Intel.
Tested with Fedora Linux guest.

[brooks]

What ever overflow I thought I saw doesn't appear to exist when I read the code now. len is always <= dst_size so the memset is safe.

I don't know what the loop is attempting to do, but all it does is compute i = MIN(strlen(src)+1, dst_size) and then i isn't used.

The only real issue I can see with this code is that pad should probably be an int like c is in memset and that's mostly theoretical.

[jhb]

I agree with @brooks that the current patch is a no-op as it is just re-computing 'strnlen' and then discarding the result.

That said, if you rewrote it to use memset of the static size first the compiler will probably inline the memset with SSE/AVX instructions and be faster than the current code. Something like:

size_t len;

len = strnlen(src, dst_size);
memset(dst, pad, dst_size);
memcpy(dst, src, len);

I would also probably make 'dst_size' a size_t. However, it would also be fine to just drop the current patch as the existing code works.

[araujo]

In D17759#379605, @brooks wrote:

What ever overflow I thought I saw doesn't appear to exist when I read the code now. len is always <= dst_size so the memset is safe.

I don't know what the loop is attempting to do, but all it does is compute i = MIN(strlen(src)+1, dst_size) and then i isn't used.

The only real issue I can see with this code is that pad should probably be an int like c is in memset and that's mostly theoretical.

Thanks @brooks, I could not see any too, but I was concerned that we could have one.

[araujo]

In D17759#379608, @jhb wrote:

I agree with @brooks that the current patch is a no-op as it is just re-computing 'strnlen' and then discarding the result.

That said, if you rewrote it to use memset of the static size first the compiler will probably inline the memset with SSE/AVX instructions and be faster than the current code. Something like:

size_t len;

len = strnlen(src, dst_size);
memset(dst, pad, dst_size);
memcpy(dst, src, len);

I would also probably make 'dst_size' a size_t. However, it would also be fine to just drop the current patch as the existing code works.

Thanks @jhb, I will close this review and open a new one with the modifications you have suggested.

[araujo]

Close in favor of: https://reviews.freebsd.org/D17795