FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

permission: address coverity warning by mhdawson · Pull Request #50215 · nodejs/node · GitHub

/ node Public

permission: address coverity warning - #50215

Closed
mhdawson wants to merge 1 commit into
nodejs:mainfrom
mhdawson:coverity40
ConversationCommits (1)ChecksFiles changed
Closed

permission: address coverity warning#50215
mhdawson wants to merge 1 commit into
nodejs:mainfrom
mhdawson:coverity40

Conversation

mhdawson commented Oct 17, 2023

Copy link
Copy Markdown
Member

The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions.

nodejs-github-bot commented Oct 17, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 17, 2023

mhdawson commented Oct 17, 2023

Copy link
Copy Markdown
Member Author

Coverity has a new set of warnings. Seeing what we think about fixing those by starting with this one. If there is a possitive response I'd plan to fix in larget baches. The warning was:

212    if (is_wildcard_node || is_last_char) {
213      std::string node_path = path.substr(parent_node_prefix_len, i);
   	
CID 329770 (#1 of 1): COPY_INSTEAD_OF_MOVE (COPY_INSTEAD_OF_MOVE)
1. copy_constructor_call: node_path is passed-by-value as parameter to CreateChild when it could be moved instead.
   	Use std::move(node_path) instead of node_path.
214      current_node = current_node->CreateChild(node_path);

RafaelGSS commented Oct 17, 2023

Copy link
Copy Markdown
Member

Coverity has a new set of warnings. Seeing what we think about fixing those by starting with this one. If there is a possitive response I'd plan to fix in larget baches. The warning was:

212    if (is_wildcard_node || is_last_char) {
213      std::string node_path = path.substr(parent_node_prefix_len, i);
   	
CID 329770 (#1 of 1): COPY_INSTEAD_OF_MOVE (COPY_INSTEAD_OF_MOVE)
1. copy_constructor_call: node_path is passed-by-value as parameter to CreateChild when it could be moved instead.
   	Use std::move(node_path) instead of node_path.
214      current_node = current_node->CreateChild(node_path);

Which compiler are you using? I didn't get these warnings building on macOS

➜  nodejs.org git:(v21-release) ✗ clang -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)

mhdawson commented Oct 17, 2023

Copy link
Copy Markdown
Member Author

@RafaelGSS they are not from the compiler from but our Coverity static analysis - https://scan.coverity.com/projects/node-js

bnoordhuis reviewed Oct 18, 2023
View reviewed changes
Comment thread src/permission/fs_permission.cc Outdated

bnoordhuis Oct 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This doesn't really change anything (except maybe shut up coverity) because CreateChild takes a value, not a reference. What you want instead is this:

diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h
index 244e95727a..80374a08c7 100644
--- a/src/permission/fs_permission.h
+++ b/src/permission/fs_permission.h
@@ -31,7 +31,7 @@ class FSPermission final : public PermissionBase {
 
       Node() : wildcard_child(nullptr), is_leaf(false) {}
 
-      Node* CreateChild(std::string prefix) {
+      Node* CreateChild(const std::string& prefix) {
         if (prefix.empty() && !is_leaf) {
           is_leaf = true;
           return this;

Or Node* CreateChild(std::string&& prefix) { + std::move(...). Look at other call sites to determine which is most appropriate.

mhdawson commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

@bnoordhuis thanks, updated

RafaelGSS commented Oct 25, 2023

Copy link
Copy Markdown
Member

@mhdawson can you rebase?

permission: address coverity warning
1c7f654 
The latest version of coverity has suggestions on how
to improve formance. Address one of these suggestions.

Signed-off-by: Michael Dawson <midawson@redhat.com>

mhdawson commented Oct 25, 2023

Copy link
Copy Markdown
Member Author

@RafaelGSS rebased on squashed commits

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Oct 27, 2023
Open
19 tasks
mhdawson added a commit that referenced this pull request Oct 27, 2023
permission: address coverity warning
3c1b4b3 
The latest version of coverity has suggestions on how
to improve formance. Address one of these suggestions.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #50215
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

mhdawson commented Oct 27, 2023

Copy link
Copy Markdown
Member Author

Landed in 3c1b4b3

mhdawson closed this Oct 27, 2023
This was referenced Oct 28, 2023
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 11, 2023
permission: address coverity warning
7c35055 
The latest version of coverity has suggestions on how
to improve formance. Address one of these suggestions.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #50215
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos mentioned this pull request Nov 12, 2023
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
permission: address coverity warning
7a661d7 
The latest version of coverity has suggestions on how
to improve formance. Address one of these suggestions.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #50215
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
UlisesGascon mentioned this pull request Dec 12, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments
benjamingr benjamingr approved these changes
RafaelGSS RafaelGSS approved these changes
marco-ippolito marco-ippolito approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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

Back | FazBrowse Home | New Git URL