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

permission: move PrintTree into unnamed namespace #48874

Merged

Conversation

tniessen
Copy link
Member

This function is not declared outside of fs_permission.cc and thus should not be visible outside the file during the linking stage.

This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot 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 Jul 21, 2023
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM. Would you mind explaining the current problem? Just to not make the same mistake again

@tniessen
Copy link
Member Author

@RafaelGSS Of course :)

When building a C++ application, each compilation unit (e.g., fs_permission.cc) is compiled separately. Then, the linker combines the resulting object files (such as fs_permission.o) into a single executable file (or static/shared library).

As a rule of thumb, the visibility of symbols during the compilation phase should match the visibility of symbols during the linking phase. In this case, for example, PrintTree is not declared in any header files, so during the compilation phase, it is only visible within a single compilation unit (i.e., within fs_permission.cc). However, during the linking phase, the PrintTree symbol will be visible regardless of where it was defined, and regardless of whether it has been declared elsewhere.

This can lead to subtle bugs. (I'll ignore name mangling in this section.)

  • For example, if someone defined another function PrintTree in child_process_permission.cc, it would not result in a compiler error because the compiler only considers one compilation unit at a time, but it would lead to an error during the linking phase. And linker errors tend to be difficult to analyze, so a compiler error is almost always preferable.
  • A worse scenario arises when someone declares a function by the same name PrintTree elsewhere but does not define it properly (e.g., because the definition contains a typo in the function name, such as PrinTre). It again won't lead to a compiler error because the compiler has seen a valid declaration of PrintTree, and it also won't lead to an error during the linking phase because the linker finds a definition of the function. The linker does not care what compilation unit the definition came from, it only cares about the name of the symbol. This might lead one compilation unit to accidentally refer to a function that was never supposed to be visible outside of another compilation unit.

Moving a function into an anonymous namespace within a compilation unit ensures that it will not be visible outside of the compilation unit during the linking phase. (In most cases, the same can be accomplished by marking the function as static, but in modern C++, we have a slight preference towards anonymous namespaces.)

Properly managing visibility has other benefits, too. For example, you might want to get a compiler warning if a function is unused. However, if a function is not in an anonymous namespace and not marked as static, the compiler has to assume that it may be referenced by other compilation units, and thus cannot detect if it is unused. Making the compiler aware of the intended visibility allows better analysis of the source code.

@RafaelGSS
Copy link
Member

Thank you!

@debadree25 debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 23, 2023
Copy link
Member

@debadree25 debadree25 left a comment

Choose a reason for hiding this comment

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

Thank you much for explaining so well @tniessen :-)

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit 88ab2e7 into nodejs:main Jul 23, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 88ab2e7

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@tniessen tniessen added the permission Issues and PRs related to the Permission Model label Aug 10, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: nodejs#48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
This function is not declared outside of fs_permission.cc and thus
should not be visible outside the file during the linking stage.

PR-URL: #48874
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants