Skip to content

crypto: fix return type prob reported by coverity#42135

Closed
mhdawson wants to merge 3 commits intonodejs:masterfrom
mhdawson:coverity-13
Closed

crypto: fix return type prob reported by coverity#42135
mhdawson wants to merge 3 commits intonodejs:masterfrom
mhdawson:coverity-13

Conversation

@mhdawson
Copy link
Copy Markdown
Member

@mhdawson mhdawson commented Feb 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]

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 25, 2022

Review requested:

  • @nodejs/crypto

@mhdawson
Copy link
Copy Markdown
Member Author

mhdawson commented Feb 25, 2022 •
edited by RaisinTen
Loading

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
Copy Markdown
Member Author

mhdawson commented Feb 25, 2022

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

@mhdawson
crypto: fix return type prob reportec by coverity
0f0987a 
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
Copy Markdown
Member Author

mhdawson commented Feb 25, 2022

Fixed lint issues and force pushed.

tniessen
tniessen approved these changes Feb 26, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 28, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42818/

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 28, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42819/

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 28, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42821/

@mhdawson
Copy link
Copy Markdown
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
Copy Markdown
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
@mhdawson
crypto: fix return type prob reported by coverity
a9c0689 
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]>
This was referenced Mar 8, 2022
Open
Open
@bengl
Copy link
Copy Markdown
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.

@BethGriggs BethGriggs mentioned this pull request Apr 9, 2022
Merged
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@mhdawson @xtx1130
crypto: fix return type prob reported by coverity
c0c18f9 
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
Copy Markdown
Member

juanarbol commented May 28, 2022

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

Reviewers

@tniessen tniessen tniessen approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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