Skip to content

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
MemberAuthor

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-botnodejs-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
MemberAuthor

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]>
@mhdawsonmhdawson changed the title crypto: fix return type prob reportec by coveritycrypto: fix return type prob reported by coverityFeb 25, 2022
@mhdawson
Copy link
MemberAuthor

Fixed lint issues and force pushed.

Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
MemberAuthor

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
MemberAuthor

Landed in a9c0689

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

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 freeto 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++.cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@mhdawson@nodejs-github-bot@bengl@juanarbol@tniessen@RaisinTen