Potential UB in secp256k1_der_parse_integer?

[practicalswift]

Do we consider memcpy(ra + 32 - rlen, *sig, rlen) in secp256k1_der_parse_integer to be defined in the case of rlen == 0?

secp256k1/src/ecdsa_impl.h

Lines 102 to 104 in 98dac87

	static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char **sig, const unsigned char *sigend) {
	int overflow = 0;
	unsigned char ra[32] = {0};

secp256k1/src/ecdsa_impl.h

Line 143 in 98dac87

memcpy(ra + 32 - rlen, *sig, rlen);

Nothing high priority of course, but perhaps worth fixing? :)

This code was introduced as part of PR #334 ("Overhaul ECDSA signature parsing: strict DER, compact sigs, tests, lower-S") in 3bb9c44 back in 2015.

[sipa]

I don't think there is any problem with that. memcpy is UB when called with invalid pointers even if length==0, but in this case I think the pointers are always valid.

Edit: you're right, this is technically UB. You can't pass pointers to one-past-the-end of an array to memcpy.

[roconnor-blockstream]

@sipa I'm not sure how you came to your conclusion:

7.1.4.1 If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined. If a function argument is described as being an array, the pointer actually passed to the function shall have a value such that all address computations and accesses to objects (that would be valid if the pointer did point to the first element of such an array) are in fact valid.

7.24.1 but in all cases a charor voidargument points to the initial (lowest addressed) character of the array. If an array is accessed beyond the end of an object, the behavior is undefined.

The pointer being passed here is both a valid value, and all address computations and accesses to objects (of which there are none) are valid.

By my reading, there is nothing wrong with the code. That said, while don't think there is a need to change anything here, I'm not going to oppose anything.

[sipa]

I consider all of this language-lawyering with very little impact on real systems, but at least here people argue that it violates the standard: https://stackoverflow.com/questions/29844298/is-it-legal-to-call-memcpy-with-zero-length-on-a-pointer-just-past-the-end-of-an

[roconnor-blockstream]

The argument in your link seems fair.

[practicalswift]

[…] at least here people argue that it violates the standard: https://stackoverflow.com/questions/29844298/is-it-legal-to-call-memcpy-with-zero-length-on-a-pointer-just-past-the-end-of-an

This thread on the musl libc mailing list is also worth reading.

Participants include:

• Pascal Cuoq (author of Frama-C)
• Rich Felkner (author of musl libc: the standards-conformant libc implementation)
• Jens Gustedt (co-editor of the ISO C standard and author of the book Modern C)