Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs. #937

pull roconnor-blockstream wants to merge 5 commits into bitcoin-core:master from roconnor-blockstream:20210504_ge_set_gej_var changing 2 files +22 −8

roconnor-blockstream commented at 7:03 PM on May 4, 2021: contributor

Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.

Have secp256k1_ge_set_gej_var initialize all fields.

Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.

dd6c3de322

gmaxwell commented at 7:07 PM on May 4, 2021: contributor

utACK dd6c3de322740a3054cf6a1994a38dc8f201b473
real-or-random commented at 7:28 PM on May 4, 2021: contributor

Concept ACK

Unless I'm mistaken, I think VG_UNDEFs here would be helpful. See #889 (comment).
gmaxwell commented at 7:31 PM on May 4, 2021: contributor

This function exits without leaving anything uninitialized, so nothing to vg_undef here. Aside, in the past I hadn't UNDEFed anywhere in the codebase itself, only in the tests to avoid injecting vg instrumentation in the production binary.. but we do that now for the CT tests and I think were were satisfied it was benign enough.

edit: oh I see, the set infinity could be undefing x/y. I suppose it could be. But that should be a separate PR if it gets done.
real-or-random commented at 7:34 PM on May 4, 2021: contributor

edit: oh I see, the set infinity could be undefing x/y. I suppose it could be.

Right, that's what I had in mind.

But that should be a separate PR if it gets done.

Fair point.
real-or-random approved
real-or-random commented at 7:36 PM on May 4, 2021: contributor

utACK dd6c3de322740a3054cf6a1994a38dc8f201b473

Have secp256k1_gej_double_var initialize all fields.

Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.

31c0f6de41

roconnor-blockstream renamed this:
~~Have secp256k1_ge_set_gej_var initialize all fields.~~
Have secp256k1_ge_set_gej_var and secp256k1_gej_double_var initialize all fields.
on May 4, 2021

Have secp256k1_ge_set_all_gej_var initialize all fields.

Previous behaviour would not initialize r->y values in the case where infinity is passed in.
Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity.

45b6468d7e

roconnor-blockstream renamed this:
~~Have secp256k1_ge_set_gej_var and secp256k1_gej_double_var initialize all fields.~~
Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs.
on May 4, 2021
gmaxwell commented at 11:50 PM on May 4, 2021: contributor

utACK 45b6468d7e3ed9849ed474c71e9a9479de1a77db
real-or-random commented at 10:33 AM on May 5, 2021: contributor

@roconnor-blockstream You could cherry pick the top two commits from https://github.com/real-or-random/secp256k1/commits/20210504_ge_set_gej_var

edit: I have verified that the new test fails on master.
roconnor-blockstream force-pushed on May 5, 2021
real-or-random commented at 2:43 PM on May 5, 2021: contributor

Ah damn, this was not the latest version on my branch -.- Let me push the right one. Sorry.
real-or-random commented at 3:19 PM on May 5, 2021: contributor

Now my branch should have the right commits... I somehow had lost them. (The reflog is nice sometimes. :))
roconnor-blockstream force-pushed on May 5, 2021
roconnor-blockstream force-pushed on May 5, 2021
tests: Test secp256k1_ge_set_all_gej_var for all infinity inputs 4a19668c37
tests: Improve secp256k1_ge_set_all_gej_var for some infinity inputs 14c9739a1f
gmaxwell commented at 6:14 PM on May 5, 2021: contributor

ACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
in src/tests.c:3109 in 14c9739a1f
```
3105 |          random_group_element_test(&ge[i]);
3106 | +        odd = secp256k1_fe_is_odd(&ge[i].x);
3107 | +        CHECK(odd == 0 || odd == 1);
3108 |          /* randomly set half the points to infinity */
3109 | -        if(secp256k1_fe_is_odd(&ge[i].x)) {
3110 | +        if (odd == i % 2) {
```
sipa commented at 6:35 PM on May 5, 2021:

Nit: % 2 seems superfluous here.

real-or-random commented at 7:00 PM on May 5, 2021:

Ah I guess it depends on the thing you want to test.

Before the change, the input consisted of some infinity points and some affine points with even x coordinate. After the change, the input consists of some infinity points and some (arbitrary) affine points.

sipa commented at 12:33 AM on May 6, 2021:

Well the line before is CHECK(odd == 0 || odd == 1);. The % 2 is guaranteed to not have any effect.

sipa commented at 2:18 AM on May 6, 2021:

Nevermind, I was just misreading this line as if (odd % 2) {.
sipa commented at 6:36 PM on May 5, 2021: contributor

utACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
real-or-random approved
real-or-random commented at 7:37 AM on May 6, 2021: contributor

ACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
real-or-random merged this on May 6, 2021
real-or-random closed this on May 6, 2021
jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick

Contributors

Linked (view graph)

#889 fix uninitialized read in tests