-
Notifications
You must be signed in to change notification settings - Fork 223
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
Autobuild: Work around CodeQL-induced build failures #3223
Conversation
It would be good to get this in, as I have a few other PRs to create, and would like their autobuilds to go green! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise approving as CI is green.
Co-authored-by: ann0see <[email protected]>
d305f2b
to
bebca88
Compare
mac/deploy_mac.sh
Outdated
@@ -120,7 +120,9 @@ build_installer_image() { | |||
|
|||
# Build installer image | |||
|
|||
create-dmg \ | |||
# Using sudo gets rid of CodeQL's virally infecting dylib preloads which break hdiutil's helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference for where this information can be confirmed? (i.e. site providing background explanation and details of why the fix is safe and appropriate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be safe as it should be equivalent to a new shell just executing create-dmg.
@hoffie probably did that based on testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment needs to be more reassuring about what's happening, in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference for where this information can be confirmed? (i.e. site providing background explanation and details of why the fix is safe and appropriate)
I don't think so. That CodeQL uses DYLIB injection can be observed by dumping environment variables. That environment-cleaned shells still inherit it was the result of my tests. That sudo
is a workaround (maybe due to SETUID) was found in other projects as well. That sudo-to-root is not required at all was a result of my tests (and is way better than running everything with elevated privileges by default, in my opinion).
0196abe
to
0cfb631
Compare
# When this script is run on Github's CI with CodeQL enabled, CodeQL adds dynamic library | ||
# shims via environment variables, so that it can monitor the compilation of code. | ||
# In order for these settings to propagate to compilation called via shell/bash scripts, | ||
# the CodeQL libs seem automatically to create the same environment variables in sub-shells, | ||
# even when called via 'env'. This was determined by experimentation. | ||
# Unfortunately, the CodeQL libraries are not compatible with the hdiutil program called | ||
# by create-dmg. In order to prevent the automatic propagation of the environment, we use | ||
# sudo to the same user in order to invoke create-dmg with a guaranteed clean environment. | ||
# | ||
# /System/Library/PrivateFrameworks/DiskImages.framework/Resources/diskimages-helper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
This PR introduces the fix created by @hoffie to prevent CodeQL from interfering with the operation of
create-dmg
/hdiutil
when the Github CI is building for MacOS.It uses
sudo
without changing user, to provide isolation of thecreate-dmg
step in the build process.CHANGELOG: Autobuild: Prevent CodeQL-induced build failures for MacOS
Context: Fixes an issue?
Fixes: #3207
Does this change need documentation? What needs to be documented and how?
No
Status of this Pull Request
Ready to merge.
What is missing until this pull request can be merged?
Just review.
Checklist