On 21/10/2025 13:30, Pádraig Brady wrote:
On 19/10/2025 23:57, Collin Funk wrote:
Pádraig Brady <[email protected]> writes:
That flag is a no-op
since:https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=ccfb2964and only
there to allow old code to compile.
That change was after the clone implementation. It looks like before
then the POSIX_SPAWN_USEVFORK implementation actually used vfork which
would likewise use clone with CLONE_VM.
The default settings may be just fine these days,
though I've not looked into the details.
The default works well, that is what I used in my original message.
Gnulib's version uses vfork when POSIX_SPAWN_USEVFORK is used, which
might help other systems, though.
Oh right. I suppose the attr management isn't too onerous.
I was wondering if POSIX_SPAWN_USEVFORK was always defined,
but it seems so since gnulib's spawn-h module says:
"<spawn.h> is always overridden, because of GNULIB_POSIXCHECK".
I notice that glibc's posix_spawn() reaps the child itself upon exec error
(which I confirmed with `strace -ff`) but means we only get a single error
now from the following:
# Old behavior
$ install src/install.c foo -s --strip-program=./foo
install: cannot run './foo': Permission denied
install: strip process terminated abnormally
# New behavior
$ src/ginstall src/install.c foo -s --strip-program=./foo
ginstall: cannot run './foo': Permission denied
That's fine though, and probably a slight improvement.
I'd add something in NEWS under "Improvements". Perhaps:
'install' now uses posix_spawn() to invoke the strip program more efficiently.
We can augment that message for sort(1) and split(1) if we make the change
there too.
Otherwise the patch looks good.
Actually I remembered thinking about not bothering about free'ing
the alloc from file_name_concat() since it was in the temp child process,
but that's no longer the case, so we do need to manage that.
That also made me realize we should destroy the attr for all errors too.
The attached should fix both issues and can be squashed into your patch.
cheers,
Padraig
From 61f7750edf5ab442474254fee4e06b157aca4588 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Tue, 21 Oct 2025 13:50:02 +0100
Subject: [PATCH] install: fix resource leaks
* NEWS: Mention the improvement.
* src/install.c (strip): Fix resource leaks.
---
NEWS | 2 ++
src/install.c | 28 +++++++++++++++-------------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/NEWS b/NEWS
index 5d70543a1..7641e4f6a 100644
--- a/NEWS
+++ b/NEWS
@@ -43,6 +43,8 @@ GNU coreutils NEWS -*- outline -*-
** Improvements
+ 'install' now uses posix_spawn() to invoke the strip program more efficiently.
+
numfmt now parses numbers with a non-breaking space character before a unit,
and parses numbers containing grouping characters from the current locale.
It also supports a multi-byte --delimiter character.
diff --git a/src/install.c b/src/install.c
index d65fa85f3..a82b8d9eb 100644
--- a/src/install.c
+++ b/src/install.c
@@ -505,31 +505,33 @@ strip (char const *name)
}
/* Construct the arguments to 'strip'. */
+ char *concat_name = nullptr;
char const *safe_name = name;
if (name && *name == '-')
- safe_name = file_name_concat (".", name, nullptr);
+ safe_name = concat_name = file_name_concat (".", name, nullptr);
char const *const argv[] = { strip_program, safe_name, nullptr };
/* Run 'strip'. */
pid_t pid;
int result = posix_spawnp (&pid, strip_program, nullptr, attrp,
(char * const *) argv, environ);
- if (result != 0)
- {
- error (0, result, _("cannot run %s"), quoteaf (strip_program));
- return false;
- }
- /* Wait for 'strip' to complete and emit a warning message if it fails. */
- int status;
bool ok = false;
- if (waitpid (pid, &status, 0) < 0)
- error (0, errno, _("waiting for strip"));
- else if (! WIFEXITED (status) || WEXITSTATUS (status))
- error (0, 0, _("strip process terminated abnormally"));
+ if (result != 0)
+ error (0, result, _("cannot run %s"), quoteaf (strip_program));
else
- ok = true;
+ {
+ /* Wait for 'strip' to complete, and emit a warning message on failure */
+ int status;
+ if (waitpid (pid, &status, 0) < 0)
+ error (0, errno, _("waiting for strip"));
+ else if (! WIFEXITED (status) || WEXITSTATUS (status))
+ error (0, 0, _("strip process terminated abnormally"));
+ else
+ ok = true;
+ }
+ free (concat_name);
if (attrp)
ignore_value (posix_spawnattr_destroy (&attr) == 0);
--
2.51.0
