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

ACL behavior depends on link order because of how privilege-storage.cpp and RequiredPrivilege.cpp interact #19706

Closed
bzbarsky-apple opened this issue Jun 17, 2022 · 5 comments
Labels
acl Access Control feature

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

src/app/RequiredPrivilege.cpp and src/app/util/privilege-storage.cpp both define functions with the same names; the former defines them as weak symbols, while the latter defines them as non-weak symbols.

Unfortunately, all the symbols in privilege-storage.cpp match names in RequiredPrivilege.cpp, so if the static library containing RequiredPrivilege.cpp happens to come earlier than the static library containing privilege-storage.cpp while linking, we will not end up pulling in the privilege-storage.cpp symbols, since those names will all be satisfied already.

The failure mode here is pretty insiduous: the application compiles/links correctly, but all ACL checks require "administer" privilege... which means the problem would not necessarily be caught in any of our normal testing, if people are generally testing with commissioners that do in fact have "administer" privileges.

Proposed Solution

Either stop using weak symbols or add some other function to privilege-storage.cpp that is called from somewhere and forces those symbols to be linked in. Any better options?

@bzbarsky-apple bzbarsky-apple added the acl Access Control feature label Jun 17, 2022
@andy31415
Copy link
Contributor

Could we devise a test to catch this issue (i.e. intentionally trigger it then verify our fix like the linking bit?).

@bzbarsky-apple
Copy link
Contributor Author

Do you mean a CI test or a certification test?

@bzbarsky-apple
Copy link
Contributor Author

Note also #16540.

@msandstedt
Copy link
Contributor

Is this resolved by #21220?

@bzbarsky-apple
Copy link
Contributor Author

Yes, it is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acl Access Control feature
Projects
None yet
Development

No branches or pull requests

3 participants