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: add missing return value check #56615

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mhdawson
Copy link
Member

Add return value check for call to SSL_CTX_add_client_CA to be consistent with other places it is called

Fixed unused warning in one of the static analysis tools we use at Red Hat even though it is not being reported by coverity in the configuration we run.

Add return value check for call to SSL_CTX_add_client_CA
to be consistent with other places it is called

Fixed unused warning in one of the static analysis tools we use
at Red Hat even though it is not being reported by coverity in
the configuration we run.

Signed-off-by: Michael Dawson <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jan 15, 2025
@mhdawson
Copy link
Member Author

@khardix can you pass this on and ask your new team members to validate this resolves the problem they are seeing?

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

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

Project coverage is 89.19%. Comparing base (1238f0a) to head (c77f543).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_context.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56615      +/-   ##
==========================================
- Coverage   89.20%   89.19%   -0.01%     
==========================================
  Files         662      662              
  Lines      191819   191819              
  Branches    36927    36923       -4     
==========================================
- Hits       171110   171100      -10     
+ Misses      13565    13557       -8     
- Partials     7144     7162      +18     
Files with missing lines Coverage Δ
src/crypto/crypto_context.cc 69.22% <0.00%> (-0.12%) ⬇️

... and 40 files with indirect coverage changes

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2025
@nodejs-github-bot
Copy link
Collaborator

@khardix
Copy link

khardix commented Jan 16, 2025

This was mostly discovered by @jackorp, so CC'ing him.

As for the fix, well, I do not think this addresses the main issue. Yes, our coverity run will now probably pass, but the -Werror=unused-result in node.gyp is still a ticking bomb.

The -Werror flag is generally only usable if one can guarantee that the software is always being built with exactly the same compiler. Different version of the compiler can produce different set of warnings, which will then cause the build to fail unexpectedly (as they are now errors). Add to this that the static analysis software we run apparently adds it's own set of flags and macro definitions, and the warnings-turned-to-errors can start to pile up rapidly.

So the actual "main fix" we would like to see is either to remove the -Werror flag, or to hide it behind a configuration option (IIRC there should be one already), so we can opt in and out depending on the environment that the build is currently running in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants