Copy the ’s’ parameter of _gej_rescale to avoid aliasing issues #1785

1. 
   peterdettman commented at 12:08 pm on December 11, 2025: contributor

   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).

2. Copy the 's' parameter of _gej_rescale to avoid aliasing issues 57652a3e4e
3. real-or-random added the label bug on Dec 11, 2025
4. 
   real-or-random commented at 12:54 pm on December 11, 2025: contributor

   utACK 57652a3e4e8c016701b2873f97f7bd5b4947e9fb

   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.

5. 
   hebasto commented at 4:16 pm on December 15, 2025: member

   Why not add a couple of VERIFY_CHECKs instead of introducing copying?
6. 
   sipa commented at 7:51 pm on December 15, 2025: contributor

   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.

Contributors
peterdettman real-or-random hebasto sipa

Labels
bug