Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

roconnor-blockstream
Copy link
Contributor

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

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

gmaxwell commented May 4, 2021

utACK dd6c3de

@real-or-random
Copy link
Contributor

Concept ACK

Unless I'm mistaken, I think VG_UNDEFs here would be helpful. See #889 (comment).

@gmaxwell
Copy link
Contributor

gmaxwell commented May 4, 2021

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
Copy link
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.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK dd6c3de

Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
@roconnor-blockstream roconnor-blockstream changed the title Have secp256k1_ge_set_gej_var initialize all fields. Have secp256k1_ge_set_gej_var and secp256k1_gej_double_var initialize all fields. May 4, 2021
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.
@roconnor-blockstream roconnor-blockstream changed the title 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. May 4, 2021
@gmaxwell
Copy link
Contributor

gmaxwell commented May 4, 2021

utACK 45b6468

@real-or-random
Copy link
Contributor

real-or-random commented May 5, 2021

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

@real-or-random
Copy link
Contributor

Ah damn, this was not the latest version on my branch -.- Let me push the right one. Sorry.

@real-or-random
Copy link
Contributor

Now my branch should have the right commits... I somehow had lost them. (The reflog is nice sometimes. :))

@roconnor-blockstream roconnor-blockstream force-pushed the 20210504_ge_set_gej_var branch 2 times, most recently from 7ab6b0a to 45b6468 Compare May 5, 2021 17:06
@gmaxwell
Copy link
Contributor

gmaxwell commented May 5, 2021

ACK 14c9739

src/tests.c Show resolved Hide resolved
@sipa
Copy link
Contributor

sipa commented May 5, 2021

utACK 14c9739

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 14c9739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants