trace mode: Various improvements #299
Merged
Conversation
This file contains 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
This is no longer required since we switched to the interposing setup and was removed from other locations in macports base a while ago.
The loader will ignore DYLD_INSERT_LIBRARIES on SUID/SGID binaries so we may as well not set them when running such binaries. This could potentially be extended with checks for other privileged binaries where DYLD_INSERT_LIBRARIES will not work.
The S_ISUID and S_ISGID bits are in the st_mode field of the stat struct, not in the st_flags field. This caused our check for SUID/SGID binaries to be incorrect, which caused us to copy SUID/SGID binaries into the sip-workaround directory, where they would then not run (correctly) because they did not have the permissions required.
darwintrace.dylib did occasionally crash in __darwintrace_is_in_sandbox due to stack canary violations, because the reimplementation of realpath and splitting into path components assumed that paths will not be longer than MAXPATHLEN. In practice, this assumption was wrong, and many build systems will invoke file system operations with paths longer than that. Switch to dynamically allocated strings and arrays and resize the buffers as necessary. This also simplifies the code significantly and fixes a number of corner cases that were previously not handled correctly, such as walking up past the root directory or multiple slashes in a row. With these changes, quassel builds successfully in trace mode – without them, the build abort(3)s.
Some of the corner cases (such as very long paths, for example) are hard to test manually, or at least you don't know exactly whether a specific code path was covered just from installing a port. Improve this situation by adding automated tests to darwintrace.
99% of all time spent in process_line() in tracelib.c happens in dep_check(). In dep_check(), time is spent as follows: - 15-20 % is spent determining the filesystem's case sensitivity - 50 % of the time is spent querying the port that provides the given path - 25 % of the time is used to translate the port ID into a port name - ~8 % are in the binary search of the port name in the list of dependencies In a test build of the yubico-pam port, 13884 iterations of dep_check() were run. Among those, there are only a little more than 3000 unique paths. This means that we can get a cache hit rate of about 78 % for a cache that stores previous lookup results. This affects the time spent in dep_check() during a test build of yubico-pam as follows (all numbers in return values of mach_absolute_time()): without cache: 2131768084 total 14188 calls ~ 150251 per call with cache: 1054866053 total 14188 calls ~ 74349 per call Caching thus reduces the duration by 50.5 %.
macOS Ventura seems to have broken trace mode because it kills signed processes when preload libraries are present. Fix this by re-signing binaries when copying them. I tested this on x86_64 Ventura, where it works as expected. Closes: https://trac.macports.org/ticket/66358
During debugging I have seen cases where stat(2) did leave the mtime fields uninitialized, so make sure the entire struct is zeroized before calling stat(2) so that there is at least a useful value rather than reading from unitinialized memory.
This test broke because the return code of /usr/bin/crontab changed when users do not have a crontab configured. Fix this by checking the output of the command if the command fails.
Since it becomes increasingly harder and harder to get an unsigned env binary that works as expected, include a stripped-down version of the FreeBSD env.c implementation for testing.
Let's not break CI on arm64, even though this test failure points to an actual problem. See: https://trac.macports.org/ticket/66358#comment:39
neverpanic
force-pushed
the
cal-tracemode-optimizations
branch
from
43fe658
to
01b90c5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of changes fixes a number of issues with trace mode:
DYLD_FORCE_FLAT_NAMESPACE=1which is no longer requiredDYLDenvironment variables into setuid binaries (which would be stripped anyway)