Fix ld's response file support for special files (part 2)
#132
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 a follow-up to: tpoechtrager#131 … which introduced a bug: the `st_size` field does not necessarily accurately represent the length if the file is a special file. For example, on macOS the `st_size` field appears to empirically be no larger than 64 KB for special files (presumably the size of some buffer), even if the actual input is larger than 64 KB. As a result, `ld` was truncating the input arguments to 64 KB for large response files, which defeats the purpose of response files (since they're typically used to support large command lines). More generally, this issue isn't specific to `fstat`, but rather appears to be an issue with anything that uses a file descriptor returned by `open`. For example, `read` misbehaves in the exact same way and refuses to read more than 64 KB from a special file that was opened by `open` even if you try to repeatedly `read` from the file to completion. For these special files, we don't have a good way to ascertain the length of the input because `fstat` won't work (that only works on file descriptors returned by `open`, which misbehave on special files), nor can we seek to the end to determine the length (because special files might not support rewinding the input) so the first part of this fix is to simply read from the input to the end and see how many bytes receive. This means that we can't allocate all the necessary memory we require up front and instead we need to dynamically resize the argument array as we read. The second part of this solution is to use `fopen` / `fread` / `close` on the unhappy path when `mmap` fails instead of using `open` / `read` / `close` since the latter operations misbehave on special files.
Gabriella439
added a commit
to MercuryTechnologies/nixpkgs
that referenced
this pull request
Feb 22, 2023
The motivation behind this is to alleviate the problem described in NixOS#41340. I'm not sure if this completely fixes the problem, but it eliminates one more area where we can exceed command line length limits. This is essentially the same change as in NixOS#112449, except for `ld-wrapper.sh` instead of `cc-wrapper.sh`. However, that change alone was not enough; on macOS the `ld` provided by `darwin.cctools` fails if you use process substitution to generate the response file, so I put up two PRs to fix that: tpoechtrager/cctools-port#131 tpoechtrager/cctools-port#132 … and I included a patch referencing that fix so that the new `ld-wrapper` still works on macOS.
Gabriella439
added a commit
to MercuryTechnologies/nixpkgs
that referenced
this pull request
Feb 22, 2023
Without this fix `ld` fails when response files are enabled and the command line length exceeds 64KB. For more details, see: tpoechtrager/cctools-port#132
Gabriella439
added a commit
to MercuryTechnologies/nixpkgs
that referenced
this pull request
Feb 22, 2023
The motivation behind this is to alleviate the problem described in NixOS#41340. I'm not sure if this completely fixes the problem, but it eliminates one more area where we can exceed command line length limits. This is essentially the same change as in NixOS#112449, except for `ld-wrapper.sh` instead of `cc-wrapper.sh`. However, that change alone was not enough; on macOS the `ld` provided by `darwin.cctools` fails if you use process substitution to generate the response file, so I put up two PRs to fix that: tpoechtrager/cctools-port#131 tpoechtrager/cctools-port#132 … and I included a patch referencing that fix so that the new `ld-wrapper` still works on macOS.
3 tasks
reckenrode
added a commit
to reckenrode/nixpkgs
that referenced
this pull request
May 31, 2024
This changes ld-wrapper to use a temporary file for the response file passed to ld instead of using process substitution. ld64 does not handle long command-lines when reading from the response file, which defeats the point of using a response file to handle long command-lines. cctools-port was patched to work around this, but nixpkgs is now using Apple’s source release directly instead of the port. Since it’s preferable not to patch Apple’s release heavily (to reduce the difficulty of updating to new versions and to match upstream’s behavior), use the approach that was adopted in cc-wrapper to work around issues with response files in newer versions of clang. Related PRs (cctools-port): - NixOS#213831 - tpoechtrager/cctools-port#132 Related PRs (cc-wrapper): - NixOS#245282 - NixOS#258608
reckenrode
added a commit
to reckenrode/nixpkgs
that referenced
this pull request
Jun 2, 2024
This changes ld-wrapper to use a temporary file for the response file passed to ld instead of using process substitution. ld64 does not handle long command-lines when reading from the response file, which defeats the point of using a response file to handle long command-lines. cctools-port was patched to work around this, but nixpkgs is now using Apple’s source release directly instead of the port. Since it’s preferable not to patch Apple’s release heavily (to reduce the difficulty of updating to new versions and to match upstream’s behavior), use the approach that was adopted in cc-wrapper to work around issues with response files in newer versions of clang. Related PRs (cctools-port): - NixOS#213831 - tpoechtrager/cctools-port#132 Related PRs (cc-wrapper): - NixOS#245282 - NixOS#258608
13 tasks
reckenrode
added a commit
to reckenrode/nixpkgs
that referenced
this pull request
Jul 13, 2024
This changes ld-wrapper to use a temporary file for the response file passed to ld instead of using process substitution. ld64 does not handle long command-lines when reading from the response file, which defeats the point of using a response file to handle long command-lines. cctools-port was patched to work around this, but nixpkgs is now using Apple’s source release directly instead of the port. Since it’s preferable not to patch Apple’s release heavily (to reduce the difficulty of updating to new versions and to match upstream’s behavior), use the approach that was adopted in cc-wrapper to work around issues with response files in newer versions of clang. Related PRs (cctools-port): - NixOS#213831 - tpoechtrager/cctools-port#132 Related PRs (cc-wrapper): - NixOS#245282 - NixOS#258608
reckenrode
added a commit
to reckenrode/nixpkgs
that referenced
this pull request
Jul 16, 2024
This changes ld-wrapper to use a temporary file for the response file passed to ld instead of using process substitution. ld64 does not handle long command-lines when reading from the response file, which defeats the point of using a response file to handle long command-lines. cctools-port was patched to work around this, but nixpkgs is now using Apple’s source release directly instead of the port. Since it’s preferable not to patch Apple’s release heavily (to reduce the difficulty of updating to new versions and to match upstream’s behavior), use the approach that was adopted in cc-wrapper to work around issues with response files in newer versions of clang. Related PRs (cctools-port): - NixOS#213831 - tpoechtrager/cctools-port#132 Related PRs (cc-wrapper): - NixOS#245282 - NixOS#258608
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 is a follow-up to:
#131
… which introduced a bug: the
st_sizefield does not necessarily accurately represent the length if the file is a special file. For example, on macOS thest_sizefield appears to empirically be no larger than 64 KB for special files (presumably the size of some buffer), even if the actual input is larger than 64 KB. As a result,ldwas truncating the input arguments to 64 KB for large response files, which defeats the purpose of response files (since they're typically used to support large command lines).More generally, this issue isn't specific to
fstat, but rather appears to be an issue with anything that uses a file descriptor returned byopen. For example,readmisbehaves in the exact same way and refuses to read more than 64 KB from a special file that was opened byopeneven if you try to repeatedlyreadfrom the file to completion.For these special files, we don't have a good way to ascertain the length of the input because
fstatwon't work (that only works on file descriptors returned byopen, which misbehave on special files), nor can we seek to the end to determine the length (because special files might not support rewinding the input) so the first part of this fix is to simply read from the input to the end and see how many bytes receive. This means that we can't allocate all the necessary memory we require up front and instead we need to dynamically resize the argument array as we read.The second part of this solution is to use
fopen/fread/closeon the unhappy path whenmmapfails instead of usingopen/read/closesince the latter operations misbehave on special files.