[PATCH] alpm_run_chroot: Fix the execution of pacman hooks
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman: ( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly Here's also an excerpt from /var/log/pacman.log (the stack trace was rather unusable, so I'll skip it for the sake of brevity): [ALPM] running '30-systemd-tmpfiles.hook'... [ALPM-SCRIPTLET] Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. [ALPM-SCRIPTLET] /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create Running "systemd-tmpfiles --create" manually afterwards resulted in no errors, which made the error message even more conspicuous. Thus, executing /usr/share/libalpm/hooks/30-systemd-tmpfiles.hook failed, but only when it was run from within pacman. I also saw a few people complaining about the same error message on Manjaro and Arch Linux forums, even with one complaint dating more than a few years ago, but nobody came through with a fix or workaround. After a detailed and rather lengthy investigation, it turned out that the root cause was twofold, as described below: 1) For its pacman package, Arch Linux ARM applies a patch named 0003-Revert-alpm_run_chroot-always-connect-parent2child-p.patch that reverts rather old pacman commit 1d6583a5, for an unknown reason. This patch causes the error like clockwork. 2) The code in pacman's lib/libalpm/util.c that executes the hooks by forking a child has some rather subtle bugs that allow the error to occur under certain circumstances. Regarding the first point, the patch from Arch Linux ARM creates a condition in which the file descriptor 0 is closed by calling close(0) and left closed when the executed hook has no option "NeedsTargets" specified, which is the case for the hook mentioned above, /usr/share/libalpm/hooks/30-systemd-tmpfiles.hook. As a result, the first call to open() during the execution of the hook returns 0 as the file descriptor, because 0 is the lowest currently available value. It would all go unnoticed, but systemd-tmpfiles performed assert(fd) checks in its file src/tmpfiles/tmpfiles.c, which failed because fd equaled 0. These checks seem to have been removed in the meantime, which effectively made the error go away, but the original issue still remains. Regarding the second point, function _alpm_run_chroot() that executes hooks in a fork()ed child does not execute dup2() properly, but instead executes close() followed by dup2(). The man page for dup2() clearly states that such attempts to re-implement the equivalent functionality must be avoided, as visible in this quotation: The dup2() system call performs the same task as dup(), but instead of using the lowest-numbered unused file descriptor, it uses the file descriptor number specified in newfd. In other words, the file descriptor newfd is adjusted so that it now refers to the same open file description as oldfd. If the file descriptor newfd was previously open, it is closed before being reused; the close is performed silently (i.e., any errors during the close are not reported by dup2()). The steps of closing and reusing the file descriptor newfd are performed atomically. This is important, because trying to implement equivalent functionality using close(2) and dup() would be subject to race conditions, whereby newfd might be reused between the two steps. Such reuse could happen because the main program is interrupted by a signal handler that allocates a file descriptor, or because a parallel thread allocates a file descriptor. As a result, a condition can occur in which the file descriptor 0 is closed by calling close(0), and left closed after the while loop that fails to execute dup2() because of receiving EBUSY, resulting in the original issue. On top of that, failed attempts to execute dup2() should be treated as fatal errors instead of being silently ignored. Let's improve the code to prevent the issues described in the second point, while not applying the above-mentioned Arch Linux ARM package patch fixes the issues decribed in the first point. While there, perform a minor cleanup as well, to make the formatting of the code a tiny bit more consistent. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent */ + exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno)); exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and -- 2.33.1
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code. The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[],
if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent */ + exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); }
- /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno)); exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
On 2023-08-20 09:22, Allan McRae wrote:
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code.
The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
Great, thanks. Sure, please let me know does the patch look fine and I'll trim the commit message down a bit and resubmit the patch. I just wanted to include all the details for the patch review.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent */ + exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno)); exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
On 2023-08-20 10:01, Dragan Simic wrote:
On 2023-08-20 09:22, Allan McRae wrote:
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code.
The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
Great, thanks. Sure, please let me know does the patch look fine and I'll trim the commit message down a bit and resubmit the patch. I just wanted to include all the details for the patch review.
Just went ahead and submitted version 2 of the patch, in which only the commit message is different.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent */ + exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno)); exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
On 08/20/23 at 05:22pm, Allan McRae wrote:
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code.
The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
I'll +1 the removal of the close() calls as unnecessary and possibly problematic, but I'm skeptical they could cause the error as described. Something opening a new FD 0 between close() and dup2() would still get overwritten by the dup2 and the almost immediately exec'd hook would still see the right thing. My understanding of the EBUSY error would be a race condition in the opposite direction, i.e. an open() call gets interrupted by a dup2() call from a signal handler on the FD the open() was setting up.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent
It won't go to the parent, but might be worth sending to stderr anyway, so there's at least a chance it makes it to the user.
+ exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno));
Our style guide is to wrap to 80 columns, this line is the correct one.
exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
On 2023-08-21 03:35, Andrew Gregory wrote:
On 08/20/23 at 05:22pm, Allan McRae wrote:
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code.
The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
I'll +1 the removal of the close() calls as unnecessary and possibly problematic, but I'm skeptical they could cause the error as described.
The described error was actually caused like clockwork by the additional reverts that Arch Linux ARM used to build its pacman package, and was caused very rarely without those reverts. While debugging the whole thing, I went rather deep and improved the libalpm code further, to also eliminate the unlikely cause of the error.
Something opening a new FD 0 between close() and dup2() would still get overwritten by the dup2 and the almost immediately exec'd hook would still see the right thing. My understanding of the EBUSY error would be a race condition in the opposite direction, i.e. an open() call gets interrupted by a dup2() call from a signal handler on the FD the open() was setting up.
If you agree, it should be the best to simply follow what the man page for dup2() says, [1] as described in the following quotation from the man page: If the file descriptor newfd was previously open, it is closed before being reused; the close is performed silently (i.e., any errors during the close are not reported by dup2()). The steps of closing and reusing the file descriptor newfd are performed atomically. This is important, because trying to implement equivalent functionality using close(2) and dup() would be subject to race conditions, whereby newfd might be reused between the two steps. Such reuse could happen because the main program is interrupted by a signal handler that allocates a file descriptor, or because a parallel thread allocates a file descriptor. [1] https://man.archlinux.org/man/dup2.2.en#dup2()
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent
It won't go to the parent, but might be worth sending to stderr anyway, so there's at least a chance it makes it to the user.
+ exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno));
Our style guide is to wrap to 80 columns, this line is the correct one.
exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
On 2023-08-21 03:35, Andrew Gregory wrote:
On 08/20/23 at 05:22pm, Allan McRae wrote:
On 15/7/23 02:31, Dragan Simic wrote:
During a full system update of a Manjaro ARM installation, on an ARM-based computer, of course, I spotted a highly conspicuous error message in the output of pacman:
( 3/18) Creating temporary files... Assertion 'fd' failed at src/tmpfiles/tmpfiles.c:843, function fd_set_perms(). Aborting. /usr/share/libalpm/scripts/systemd-hook: line 28: 1735 Aborted (core dumped) /usr/bin/systemd-tmpfiles --create error: command failed to execute correctly
<snip>
Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- lib/libalpm/util.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
@Andrew: any chance you can look at this? You are more familiar with this bit of code.
The justification looks fine to me (though we can really limit the commit message. Not including irrelevant Acrh ARM reverts would be a start to shortening it).
I'll +1 the removal of the close() calls as unnecessary and possibly problematic, but I'm skeptical they could cause the error as described. Something opening a new FD 0 between close() and dup2() would still get overwritten by the dup2 and the almost immediately exec'd hook would still see the right thing. My understanding of the EBUSY error would be a race condition in the opposite direction, i.e. an open() call gets interrupted by a dup2() call from a signal handler on the FD the open() was setting up.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index dffa3b51..97e87c6c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -639,28 +639,39 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - close(0); - close(1); - close(2); - while(dup2(child2parent_pipefd[HEAD], 1) == -1 && errno == EINTR); - while(dup2(child2parent_pipefd[HEAD], 2) == -1 && errno == EINTR); - while(dup2(parent2child_pipefd[TAIL], 0) == -1 && errno == EINTR); - close(parent2child_pipefd[TAIL]); close(parent2child_pipefd[HEAD]); close(child2parent_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDERR_FILENO) == -1) { + if(errno != EINTR) { + /* at this point, the child cannot talk through the parent
It won't go to the parent, but might be worth sending to stderr anyway, so there's at least a chance it makes it to the user.
I'm sorry for replying again, I somehow missed your comments on the actual patch. That's a good point, thanks, I'll create and submit version 3 of the patch, which does it that way.
+ exit(1); + } + } + while(dup2(parent2child_pipefd[TAIL], STDIN_FILENO) == -1) { + if(errno != EINTR) { + /* use fprintf() instead of _alpm_log() to send output through the parent */ + fprintf(stderr, _("could not redirect standard input (%s)\n"), strerror(errno)); + exit(1); + } + } + close(parent2child_pipefd[TAIL]); + while(dup2(child2parent_pipefd[HEAD], STDOUT_FILENO) == -1) { + if(errno != EINTR) { + fprintf(stderr, _("could not redirect standard output (%s)\n"), strerror(errno)); + exit(1); + } + } close(child2parent_pipefd[HEAD]); if(cwdfd >= 0) { close(cwdfd); } - /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } if(chdir("/") != 0) { - fprintf(stderr, _("could not change directory to %s (%s)\n"), - "/", strerror(errno)); + fprintf(stderr, _("could not change directory to %s (%s)\n"), "/", strerror(errno));
Our style guide is to wrap to 80 columns, this line is the correct one.
I see, but there are already more than a few lines longer than 80 characters in the code, such as the fprintf() invocation above, which is the only reason why I made that line longer.
exit(1); } /* bash assumes it's being run under rsh/ssh if stdin is a socket and
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Dragan Simic