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

Refactor fe_isnonzero to fe_isnonzero_vartime #1893

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tisonkun
Copy link

This refers to #1827.

An intuitive solution according to @briansmith's comment. But I know little about crypto so perhaps some invariants are broken.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
crypto/curve25519/curve25519.c Outdated Show resolved Hide resolved
crypto/mem.c Show resolved Hide resolved
src/constant_time.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 13, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.30%. Comparing base (9cb93e0) to head (407599a).
Report is 174 commits behind head on main.

Files with missing lines Patch % Lines
crypto/curve25519/curve25519.c 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1893   +/-   ##
=======================================
  Coverage   96.29%   96.30%           
=======================================
  Files         135      135           
  Lines       20663    20667    +4     
  Branches      226      228    +2     
=======================================
+ Hits        19898    19903    +5     
  Misses        730      730           
+ Partials       35       34    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tisonkun
Copy link
Author

tisonkun commented Jan 13, 2024

Thanks for your review @briansmith!

Do you think it's better to exclude changes around src/constant_time.rs in this PR and move forward, or we can wait for #1899 merged?

@briansmith
Copy link
Owner

Yes, I think we should limit this to the curve25519.c change and mem.h deletion.

@tisonkun
Copy link
Author

Updated. PTAL @briansmith.

@tisonkun tisonkun requested a review from briansmith January 17, 2024 13:45
@tisonkun
Copy link
Author

@briansmith ping as a reminder :D

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

I noticed that the code coverage for has missing coverage for one of the calls. I am investigating that.

crypto/curve25519/curve25519.c Show resolved Hide resolved
@tisonkun tisonkun requested a review from briansmith February 21, 2024 14:04
@tisonkun
Copy link
Author

tisonkun commented Mar 9, 2024

@briansmith ping as a reminder :D

@tisonkun
Copy link
Author

tisonkun commented Apr 9, 2024

@briansmith I suppose the codecov regression is false positive..

@tisonkun
Copy link
Author

tisonkun commented Jul 6, 2024

@briansmith is there other blocker to this PR?

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.

2 participants