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

[Crypto] C code sanitization #4654

Merged
merged 14 commits into from
Aug 31, 2023
Merged

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Aug 24, 2023

  • add C code sanitizers Makefile targets:
    • memory sanitizer msan (only works on Linux and with clang)
    • address sanitizer asan (only works on Linux)
    • other sanitizers (only work on Linux)
  • add sanitizers (but msan) to a CI job and they are required to pass for new PRs
  • msan has many false positive errors from use-of-unitialized-value. These errors have been debugged one by one to make sure no uninitialized memory has been read.
  • NO_MSAN macro has been added during debugging to disable msan in functions. The macro was left in the code for future debugging. It is only defined when compiling with defined MSAN (look at the c-msan target in Makefile).

Side changes:

  • Update ./bls_src/README with instructions of updating BLST version.
  • clean up non used Fermat modular inversion and Montgomery modular expo.
  • minor optimization to Fr_is_zero.
  • update DEBUG macro .

@tarakby tarakby changed the base branch from master to feature/blst-based-crypto August 24, 2023 17:34
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -7.54% ⚠️

Comparison is base (20b89d2) 61.23% compared to head (8314b0c) 53.70%.

Additional details and impacted files
@@                      Coverage Diff                      @@
##           feature/blst-based-crypto    #4654      +/-   ##
=============================================================
- Coverage                      61.23%   53.70%   -7.54%     
=============================================================
  Files                            280      857     +577     
  Lines                          28651    79948   +51297     
=============================================================
+ Hits                           17545    42937   +25392     
- Misses                          9653    33615   +23962     
- Partials                        1453     3396    +1943     
Flag Coverage Δ
unittests 53.70% <ø> (-7.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 580 files with indirect coverage changes

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

@tarakby tarakby changed the title [Crypto] WIP [Crypto] C code sanitization Aug 29, 2023
@tarakby tarakby marked this pull request as ready for review August 29, 2023 23:43
@tarakby tarakby requested a review from gomisha as a code owner August 29, 2023 23:43
@tarakby tarakby removed the request for review from gomisha August 30, 2023 00:31
@tarakby tarakby merged commit 6bb393c into feature/blst-based-crypto Aug 31, 2023
32 checks passed
@tarakby tarakby deleted the tarak/blst-sanitize branch August 31, 2023 21:18
This pull request was closed.
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