Skip to content

test: address coverity issue #44800

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

mhdawson
Copy link
Member

Coverity is reporting issues with negative
returns. I think that is because

size_t page = GetPageSize();

will always result in page being positive even if GetPageSize fails and returns -1 since size_t cannot be negative. This then would result n the check right afterwards not catching the failure.

Fix by converting to size_t after we do the check.

Signed-off-by: Michael Dawson mdawson@devrus.com

Coverity is reporting issues with negative
returns. I think that is because

size_t page = GetPageSize();

will always result in page being positive even if GetPageSize
fails and returns -1 since size_t cannot be negative. This then
would result n the check right afterwards not catching the failure.

Fix by converting to size_t after we do the check.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 26, 2022
@mhdawson
Copy link
Member Author

mhdawson commented Sep 26, 2022

Reports from coverity


*** CID 278353:  Error handling issues  (NEGATIVE_RETURNS)
/test/cctest/test_crypto_clienthello.cc: 57 in OverrunGuardedBuffer<(unsigned long)5>::OverrunGuardedBuffer()()
51         size_t page = GetPageSize();
52         EXPECT_GE(page, N);
53     #endif
54     #ifdef USE_MPROTECT
55         // Place the packet right before a guard page, which, when accessed, causes
56         // a segmentation fault.
>>>     CID 278353:  Error handling issues  (NEGATIVE_RETURNS)
>>>     "page" is passed to a parameter that cannot be negative.
57         alloc_base = static_cast<uint8_t*>(aligned_alloc(page, 2 * page));
58         EXPECT_NE(alloc_base, nullptr);
59         uint8_t* second_page = alloc_base + page;
60         EXPECT_EQ(mprotect(second_page, page, PROT_NONE), 0);
61         data_base = second_page - N;
62     #elif defined(USE_VIRTUALPROTECT)

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2022
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.

I might be missing something but I don't see how this helps. GoogleTest's EXPECT_GE does not stop execution, it just logs an error. (I didn't know this at first, which is why I used EXPECT_GE in GetPageSize(), see #44795.)

@tniessen
Copy link
Member

tniessen commented Oct 1, 2022

size_t page = GetPageSize();

will always result in page being positive even if GetPageSize fails and returns -1 since size_t cannot be negative. This then would result n the check right afterwards not catching the failure.

Fix by converting to size_t after we do the check.

That's kind of exactly what GetPageSize() does internally: assign to an int first, check that it is positive, then cast to size_t :)

@mhdawson
Copy link
Member Author

@tniessen I can't remember what I was thinking at this point, but I see your point so closing.

@mhdawson mhdawson closed this Oct 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants