C impl: Prevent memcpy undefined behavior

[guidovranken]

This prevents a (potential, depending on the caller) case of undefined behavior in the call to memcpy in chunk_state_fill_buf. This can happen when blake3_hasher_update is called with input=NULL and input_len=0. Passing a null pointer to memcpy is undefined behavior even if the n argument is 0.

Empy C++ vectors (can) behave this way, eg.:

std::vector<uint8_t> v;
blake3_hasher_update(&hasher, v.data(), v.size());

[oconnor663]

Thanks for catching this. I probably won't have time to merge it today, but I should get to it tomorrow. For now, I'll note that this code is currently only used to test the other C code; the Rust crate does not build this file.

[cemeyer]

Passing a null pointer to memcpy is undefined behavior even if the n argument is 0.

I'm not sure that's undefined behavior (but I'm also not confident it isn't). From C18, § 7.24.1 "String handling" > "String function conventions", (2):

Where an argument declared as size_t n specifies the length of the array for a function, n can have
the value zero on a call to that function. … On such a call, ... a function that copies characters copies zero characters.

A few paragraphs later, § 7.24.2.1, "The memcpy function", (2):

The memcpy function copies n characters from the object pointed to by s2 into the object pointed to by s1 .

I don't really understand how any valid implementation of memcpy could produce UB regardless of s1 and s2, if n is 0.

[oconnor663]

@cemeyer take a look at https://www.imperialviolet.org/2016/06/26/nonnull.html

[cemeyer]

@oconnor663 It's a great write-up.

I think perhaps Langley and definitely the glibc authors have interpreted §7.1.4 differently than I have. The language only says,

If an argument to a function has an invalid value (such as ... or a null pointer, or ….) …, the behavior is undefined.

The sentence includes other examples clearly irrelevant to string functions, such as mathematical values out of domain. So I believe the "such as" language is intended to provide only examples of possible invalid argument values, rather than a list of values which are always invalid. Indeed, some of the string.h functions explicitly permit NULL arguments, so it cannot be the case that the entire list is always invalid and therefore use in arguments is always UB.

So the question I think is, is NULL an invalid value for s1 or s2 of any valid implementation of memcpy, with n=0? And I don't see any reason to believe that. If n is zero, it is completely bogus for memcpy to dereference s1 or s2, and thus it does not matter if they are valid object pointers.

Similarly, it is valid for malloc(0) to return a non-null pointer. But it is UB to attempt to access the object pointed to. Such a pointer would be a valid input to memcpy(,, 0), but not a valid input with any non-zero size, despite being non-NULL. NULL is not what makes a memcpy argument valid or invalid; it is a combination of the value of n, as well as the size of the object pointed to by s1 and s2 (or NULL, which is essentially the same as a theoretical non-null pointer returned by malloc(0)).

In response to Langley's example of glibc, nonnull, and compiler optimizations, I think basically glibc was wrong to annotate memcpy with the nonnull attribute. (And perhaps GCC was wrong to provide the attribute in the first place — the name is misleading and leads to accidental misuse, the optimizations from the annotation that are not provided by other analysis are extremely limited, and the downside is large.) The optimization bugs are fallout from that erroneous annotation.

As Langley goes on to describe, there is negligible to zero program code benefit from making this invocation UB, and potentially large downside. I think I'm largely on the same page with his Conclusions section.

Although I've little hope of it happening, I'd suggest that GCC and glibc remove these assumptions and that the next revision of the C standard change 7.24.1(2) to clarify that when a length is zero, pointers can be NULL.

💯

In summary: I am still not persuaded this is actually UB per the standard. However, please go ahead and make this change to defend against the glibc/GCC implementation-specific foot-gun.

[oconnor663]

Merged and followed on with d7d71b2.