undefined behaviour in memcpy_s

[kloetzl]

Hi,

In memcpy_s there is a guard in place to prevent copying overlapping ranges (which is UB). However, the way the check is implemented it will trigger UB in all other cases. Comparing pointers from two separately allocated objects is forbidden as per 6.5.8 C11 except when using (in)equality. I suggest using a cast to uintptr_t as seen in mem_prim_move.

Best,
Fabian

/edit: Accidentally wrote memset_s at first.

[kloetzl]

Thinking about it I came to the conclusion that you are also handling the problem badly. Using memcpy with overlapping ranges is UB. But that means that you can define the behaviour to make it perfectly valid. Thus, instead of bailing out on an overlap, print a warning and just call memmove.

[dmwheel1]

@kloetzl - thanks for bringing this up. Indeed the use of uintptr would be better.
However, the difference between memmove and memcpy has to do with the expectations of the user.
memcpy specifically has a constraint that moves cannot be done if the regions overlap. This is to protect the user in case they make a mistake. If the user actually wants to copy even if the memory regions overlap, then they should use memmove. This is actually the reason for having two different interfaces to essentially the same function.

[kloetzl]

memcpy specifically has a constraint that moves cannot be done if the regions overlap.

That is true for memcpy, but not for memcpy_s. In the case of overlapping memory regions the latter nulls the memory as per the spec; which I think is an error. It could simply forward to memmove. However, as you just want to implement Annex K, your behavior is correct (expect except for the UB).

[dmwheel1]

Sorry but I am not sure that I understand the statement:
"However, as you just want to implement Annex K, your behavior is correct (expect for the UB)."
What do you mean by "expect for the UB"?
My C11 standard (Version April 12, 2011) for memcpy_s (in K.3.7.1.1 paragraph 2 & 3) states:
"2 Neither s1 nor s2 shall be a null pointer. Neither s1max nor n shall be greater than RSIZE_MAX.
n shall not be greater than s1max. Copying shall not take place between objects that overlap."
"3 If there is a runtime-constraint violation, the memcpy_s function stores zeros in the first s1max
characters of the object pointed to by s1 if s1 is not a null pointer and s1max is not greater
than RSIZE_MAX"

The library allows a runtime time setting to set the constraint handler - if that is not set (== NULL)
then the ignore handler is called which does not perform any NULL'ing of memory. This is my
preference as it seems to me that I never want someone NULL'ing my source string.

Is this to what you are referring? I don't quite understand "UB" though - can you explain?

[kloetzl]

That should have been “except”, my bad.

Is this to what you are referring? I don't quite understand "UB" though - can you explain?

The following comparison is undefined.

safestringlib/safeclib/memcpy_s.c

Lines 148 to 149 in 38c0fa9

	if( ((dp > sp) && (dp < (sp+smax))) ||
	((sp > dp) && (sp < (dp+dmax))) ) {

[dmwheel1]

Thanks. I'll submit a fix this week for this one, and will look for other instances (unless you happen to know of any).