permission: move PrintTree into unnamed namespace

[tniessen]

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]

Review requested:

• @nodejs/security-wg

[RafaelGSS]

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

[tniessen]

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

Thank you!

[debadree25]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 88ab2e7