[PATCH] qemuNamespaceMknodOne: Call g_file_read_link() in async-signal-safe fashion

Michal Privoznik posted 1 patch 1 year, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/36baf29ce3c028dcfdfe3a18b21c356cee31be83.1664520681.git.mprivozn@redhat.com
src/qemu/qemu_namespace.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH] qemuNamespaceMknodOne: Call g_file_read_link() in async-signal-safe fashion
Posted by Michal Privoznik 1 year, 6 months ago
When creating a node in QEMU's namespace the whole link chain is
created with it. Here, we use g_file_read_link() from the child
(running inside the namespace) to learn whether a link exists and
points to expected target. Now, when building the namespace there
can't be any symlinks and this g_file_read_link() returns NULL
always. And because we pass a local GError variable to it, glib
tries to set it to a localized error message. This comes with
creating a (static) hash table inside of g_strerror() and is
guarded with a mutex. The hash table is also allocated using
GSlice allocator instead of g_malloc, and since the latter is
safe to use after fork (because it's documented to use plain
malloc), glib went with the former, naturally. Now, GSlice
allocator has plenty of internal mutexes and thus hitting a
locked mutex is not that hard.

Fortunately, we don't care about any error from
g_file_read_link() and thus we can pass NULL which avoids calling
g_strerror().

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2120965
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_namespace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index a2c31310d9..311c66d46e 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -957,10 +957,9 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
     }
 
     if (isLink) {
-        g_autoptr(GError) gerr = NULL;
         g_autofree char *target = NULL;
 
-        if ((target = g_file_read_link(data->file, &gerr)) &&
+        if ((target = g_file_read_link(data->file, NULL)) &&
             STREQ(target, data->target)) {
             VIR_DEBUG("Skipping symlink %s -> %s which exists and points to correct target",
                       data->file, data->target);
-- 
2.35.1
Re: [PATCH] qemuNamespaceMknodOne: Call g_file_read_link() in async-signal-safe fashion
Posted by Jiri Denemark 1 year, 6 months ago
On Fri, Sep 30, 2022 at 08:51:21 +0200, Michal Privoznik wrote:
> When creating a node in QEMU's namespace the whole link chain is
> created with it. Here, we use g_file_read_link() from the child
> (running inside the namespace) to learn whether a link exists and
> points to expected target. Now, when building the namespace there
> can't be any symlinks and this g_file_read_link() returns NULL
> always. And because we pass a local GError variable to it, glib
> tries to set it to a localized error message. This comes with
> creating a (static) hash table inside of g_strerror() and is
> guarded with a mutex. The hash table is also allocated using
> GSlice allocator instead of g_malloc, and since the latter is
> safe to use after fork (because it's documented to use plain
> malloc), glib went with the former, naturally. Now, GSlice
> allocator has plenty of internal mutexes and thus hitting a
> locked mutex is not that hard.
> 
> Fortunately, we don't care about any error from
> g_file_read_link() and thus we can pass NULL which avoids calling
> g_strerror().
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2120965
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_namespace.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index a2c31310d9..311c66d46e 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -957,10 +957,9 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
>      }
>  
>      if (isLink) {
> -        g_autoptr(GError) gerr = NULL;
>          g_autofree char *target = NULL;
>  
> -        if ((target = g_file_read_link(data->file, &gerr)) &&
> +        if ((target = g_file_read_link(data->file, NULL)) &&
>              STREQ(target, data->target)) {
>              VIR_DEBUG("Skipping symlink %s -> %s which exists and points to correct target",
>                        data->file, data->target);

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>