[Libguestfs] Re: [PATCH] deamon: btrfs: Fix invalid memory access
On Thu, Nov 20, 2025 at 01:25:54PM +0200, Arye Yurkovsky wrote:
---
daemon/btrfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 4b6059621..161cae3cd 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -971,11 +971,11 @@ do_btrfs_subvolume_show (const char *subvolume)
return NULL;
if (ss_len != 0)
- ss[ss_len++] = ',';
+ ss[ss_len - 1] = ',';
memcpy (ss + ss_len, key, strlen (key));
ss_len += strlen (key);
- ss[ss_len] = '\0';
+ ss[ss_len++] = '\0';
p = analyze_line (p, &key, &value, ':');
}
Thinking about the original code, let's say we have keys which are
10 and 5 bytes long.
On the first iteration, ss_len == 0, so we allocate the buffer ss as 0
+ 10 + 1 == 11 bytes. We then copy the key (10 bytes), set ss_len = 10,
and set ss[10] = '\0'.
On the second iteration, ss_len == 10, so we reallocate the buffer ss
to 10 + 5 + 1 == 16 bytes. We then set ss[10+1] = ',' (wrong?) and
increment ss_len to 11. We then copy the key (5 bytes) to ss[11..15].
We then set ss[16] = '\0' (which is beyond the end of the allocated
array).
Thinking about your proposed code, with the same keys.
The first iteration is the same as above, except that ss_len becomes
11 after the iteration.
On the second iteration, ss_len == 11, so we reallocate the buffer ss
to 11 + 5 + 1 == 17 bytes. We then set ss[11-1] = ',' which I think
is correct. We copy the key (5 bytes) to ss[11..15]. We then set
ss[17] = '\0' and increment ss_len, and I think that's wrong.
This code (both the original and with the patch) is hurting my head so
I'm going to suggest a different rewrite for it.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top