Re: [PATCH] alpm_run_chroot: Fix the execution of pacman hooks

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 --- 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