From 56da6b36d3cb10711b4c6a24930e1bbc2cd5f38b Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Fri, 21 Nov 2025 17:19:29 +0000 Subject: [PATCH] daemon: btrfs: Simplify snapshot code and fix invalid memory access The existing code had a bug which you can demonstrate by doing: $ guestfish -N fs:btrfs:10G -m /dev/sda1 \ btrfs-subvolume-create /sub : btrfs-subvolume-snapshot /sub /snap1 : \ btrfs-subvolume-snapshot /sub /snap123 : \ btrfs-subvolume-snapshot /sub /snap123456 : \ btrfs-subvolume-show /sub ... libguestfs: error: appliance closed the connection unexpectedly. This usually means the libguestfs appliance crashed. As the code for parsing the output and creating the comma-separated list of snapshots was unncessarily complicated in the first place, simplify it. This also fixes the bug. This also adds a regression test. Thanks: Arye Yurkovsky Link: https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/QV5VDHIH7WRUNAE54K6OEOKJMWL6M7EM/ --- daemon/btrfs.c | 31 ++++----- tests/Makefile.am | 6 +- tests/btrfs/test-btrfs-subvolume-snapshot.sh | 67 ++++++++++++++++++++ 3 files changed, 82 insertions(+), 22 deletions(-) create mode 100755 tests/btrfs/test-btrfs-subvolume-snapshot.sh diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 4b6059621..173726fd2 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -957,8 +957,8 @@ do_btrfs_subvolume_show (const char *subvolume) while (key) { /* snapshot is special, see the output above */ if (STREQLEN (key, "Snapshot(s)", sizeof ("Snapshot(s)") - 1)) { - char *ss = NULL; - int ss_len = 0; + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (snapshots); + char *ss; if (add_string (&ret, key) == -1) return NULL; @@ -966,27 +966,18 @@ do_btrfs_subvolume_show (const char *subvolume) p = analyze_line (p, &key, &value, ':'); while (key && !value) { - ss = realloc (ss, ss_len + strlen (key) + 1); - if (!ss) - return NULL; - - if (ss_len != 0) - ss[ss_len++] = ','; - - memcpy (ss + ss_len, key, strlen (key)); - ss_len += strlen (key); - ss[ss_len] = '\0'; - + if (add_string (&snapshots, key) == -1) + return NULL; p = analyze_line (p, &key, &value, ':'); } - if (ss) { - if (add_string_nodup (&ret, ss) == -1) - return NULL; - } else { - if (add_string (&ret, "") == -1) - return NULL; - } + if (end_stringsbuf (&snapshots) == -1) + return NULL; + + /* Turn the snapshots into a comma-separated list. */ + ss = guestfs_int_join_strings (",", snapshots.argv); + if (add_string_nodup (&ret, ss) == -1) + return NULL; } else { if (add_string (&ret, key) == -1) return NULL; diff --git a/tests/Makefile.am b/tests/Makefile.am index c7ecbc9eb..b2cf1e698 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -50,12 +50,14 @@ TESTS += \ btrfs/test-btrfs-misc.pl \ btrfs/test-btrfs-devices.sh \ btrfs/test-btrfs-subvolume-default.pl \ - btrfs/test-btrfs-replace.sh + btrfs/test-btrfs-replace.sh \ + btrfs/test-btrfs-subvolume-snapshot.sh EXTRA_DIST += \ btrfs/test-btrfs-misc.pl \ btrfs/test-btrfs-devices.sh \ btrfs/test-btrfs-subvolume-default.pl \ - btrfs/test-btrfs-replace.sh + btrfs/test-btrfs-replace.sh \ + btrfs/test-btrfs-subvolume-snapshot.sh CLEANFILES += \ c-api/test.log \ diff --git a/tests/btrfs/test-btrfs-subvolume-snapshot.sh b/tests/btrfs/test-btrfs-subvolume-snapshot.sh new file mode 100755 index 000000000..e666d7511 --- /dev/null +++ b/tests/btrfs/test-btrfs-subvolume-snapshot.sh @@ -0,0 +1,67 @@ +#!/bin/bash - +# libguestfs +# Copyright Red Hat +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test btrfs subvolumes and snapshots. + +source ./functions.sh +set -e +set -x + +skip_if_skipped +skip_unless_feature_available btrfs + +guestfish <<'EOF' + +# Create a large empty disk. +sparse test-btrfs-subvolume-snapshot.img 10G +run + +mkfs btrfs /dev/sda +mount /dev/sda / + +# Create some subvolumes. +btrfs-subvolume-create /sub0 +btrfs-subvolume-create /sub1 +btrfs-subvolume-create /sub3 + +# Create a few snapshots. +btrfs-subvolume-snapshot /sub1 /snap11 +btrfs-subvolume-snapshot /sub3 /snap31 +btrfs-subvolume-snapshot /sub3 /snap3123 +btrfs-subvolume-snapshot /sub3 /snap3123456123456 + +# List the subvolumes. +btrfs-subvolume-show /sub0 +btrfs-subvolume-show /sub1 +btrfs-subvolume-show /sub3 + +# Capture the list of snapshots. +btrfs-subvolume-show /sub0 | grep -F 'Snapshot(s):' > subvolume-snapshot.out +btrfs-subvolume-show /sub1 | grep -F 'Snapshot(s):' >> subvolume-snapshot.out +btrfs-subvolume-show /sub3 | grep -F 'Snapshot(s):' >> subvolume-snapshot.out + +EOF + +cat subvolume-snapshot.out + +test "$(cat subvolume-snapshot.out)" = \ + "Snapshot(s): +Snapshot(s): snap11 +Snapshot(s): snap31,snap3123,snap3123456123456" + +rm test-btrfs-subvolume-snapshot.img subvolume-snapshot.out