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: fix return type prob reported by coverity #42135

Closed
wants to merge 3 commits into from

Conversation

mhdawson
Copy link
Member

Coverity correctly reported that the value returned
by BIO_get_mem_data could be negative and the type
provided for the return value was unsigned.

Fix up the type and check.

Signed-off-by: Michael Dawson [email protected]

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@mhdawson
Copy link
Member Author

mhdawson commented Feb 25, 2022

This is what was reported by Coverity

1 new defect(s) introduced to Node.js found with Coverity Scan.


New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 249964:  Integer handling issues  (NEGATIVE_RETURNS)


________________________________________________________________________________________________________
*** CID 249964:  Integer handling issues  (NEGATIVE_RETURNS)
/src/crypto/crypto_common.cc: 772 in node::crypto::PrintGeneralName(const std::unique_ptr<bio_st, node::FunctionDeleter<bio_st, (&BIO_free_all)>> &, const GENERAL_NAME_st *)()
766                                kX509NameFlagsRFC2253WithinUtf8JSON) < 0) {
767           return false;
768         }
769         char* oline = nullptr;
770         size_t n_bytes = BIO_get_mem_data(tmp.get(), &oline);
771         CHECK_IMPLIES(n_bytes != 0, oline != nullptr);
>>>     CID 249964:  Integer handling issues  (NEGATIVE_RETURNS)
>>>     "n_bytes" is passed to a parameter that cannot be negative.
772         PrintAltName(out, oline, n_bytes, true, nullptr);
773       } else if (gen->type == GEN_IPADD) {
774         BIO_printf(out.get(), "IP Address:");
775         const ASN1_OCTET_STRING* ip = gen->d.ip;
776         const unsigned char* b = ip->data;
777         if (ip->length == 4) {

@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 Feb 25, 2022
@mhdawson
Copy link
Member Author

I don't know whey I don't get the linter errors when I run locally :(

Coverity correctly reported that the value returned
by BIO_get_mem_data could be negative and the type
provided for the return value was unsigned.

Fix up the type and check.

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson mhdawson changed the title crypto: fix return type prob reportec by coverity crypto: fix return type prob reported by coverity Feb 25, 2022
@mhdawson
Copy link
Member Author

Fixed lint issues and force pushed.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

FWIW, BIO_get_mem_data internally casts a size_t to long, so the result is guaranteed to be valid when cast to size_t.

src/crypto/crypto_common.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member Author

mhdawson commented Mar 1, 2022

The update to the checks on the issue seems to be stale. This CI run - https://ci.nodejs.org/job/node-test-pull-request/42821/ shows all green. Going to land.

@mhdawson
Copy link
Member Author

mhdawson commented Mar 1, 2022

Landed in a9c0689

@mhdawson mhdawson closed this Mar 1, 2022
mhdawson added a commit that referenced this pull request Mar 1, 2022
Coverity correctly reported that the value returned
by BIO_get_mem_data could be negative and the type
provided for the return value was unsigned.

Fix up the type and check.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #42135
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@bengl
Copy link
Member

bengl commented Mar 21, 2022

This doesn't land on 17.x, and it looks like it's because it's modifying code added in a semver-major, so I'm adding the dont-land-on-17 label.

xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Coverity correctly reported that the value returned
by BIO_get_mem_data could be negative and the type
provided for the return value was unsigned.

Fix up the type and check.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#42135
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@juanarbol
Copy link
Member

This depends on #42002 which is another semver major; so this can not be landed in V16.x

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.

6 participants