Copy the 's' parameter of _gej_rescale to avoid aliasing issues

[peterdettman]

There doesn't appear to be any reason why a caller of _gej_rescale couldn't pass an s that aliases one of the fields of r. In case it aliased r->y or r->z, at least a VERIFY_CHECK in _fe_mul could catch the problem. However if r->x were aliased, in general an invalid result would be silently produced, due to s being read after r->x is written.

This PR proposes simply copying s to a local at the start of the method. (Also updates the group.h method decl so that the parameter names match).

[real-or-random]

utACK 57652a3 (edit: The change is correct, but I believe we should do something else, see below.)

Urghs, great catch.

Now that I see this, I believe this is a larger (latent) problem. In most functions, entire ges could alias, and I doubt the code has always been written with this in mind. (And the same in the field and scalar code.) Not sure what this means. Perhaps we should add VERIFY_CHECKs everywhere, and postpone the actual fixing of the operations when we want to add code that runs into these VERIFY_CHECKs. (I assume this is how you found this one.) Also because the overhead introduced by copying may sometimes be noticeable.

[hebasto]

Why not add a couple of VERIFY_CHECKs instead of introducing copying?

[sipa]

I think we should decide whether to allow aliasing, or not, of this argument:

• If we allow aliasing, the function should be changed like this PR does, but I think it should be accompanied with a test that exercises that aliasing, so it doesn't accidentally get broken later.
• If we don't allow aliasing, we should mark the argument as SECP256K1_RESTRICT, and add corresponding VERIFY_CHECK.

I have not checked the compilation output, but my guess is that both options will actually generate (very slightly) more performant code than current master, as in both cases the compiler can rely on the assumption that the scale value (or copy thereof) doesn't change throughout the execution.

[real-or-random]

• If we allow aliasing, the function should be changed like this PR does, but I think it should be accompanied with a test that exercises that aliasing, so it doesn't accidentally get broken later.
• If we don't allow aliasing, we should mark the argument as SECP256K1_RESTRICT, and add corresponding VERIFY_CHECK.

Agreed, and I'd prefer the second one, just because it's less work. Exceptions can be made in the future if we actually need them.