[libvirt PATCH] storage: logical: don't write past buffer bounds
From: Ján Tomko <jtomko@redhat.com> If parsed number of extents is 0, the calculated buffer size won't hold the first regex_unit. Closes: https://gitlab.com/libvirt/libvirt/-/work_items/913 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 51e9337820..81f9d62d8b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -161,12 +161,12 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDef *vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ regex = g_new0(char, nextents * (strlen(regex_unit) + 1) + 1); - strcat(regex, regex_unit); - for (i = 1; i < nextents; i++) { + for (i = 0; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); strcat(regex, regex_unit); + strcat(regex, ","); } + regex[strlen(regex) - 1] = '\0'; re = g_regex_new(regex, 0, 0, &err); if (!re) { -- 2.55.0
On 8/21/26 12:51, Ján Tomko via Devel wrote:
From: Ján Tomko <jtomko@redhat.com>
If parsed number of extents is 0, the calculated buffer size won't hold the first regex_unit.
Closes: https://gitlab.com/libvirt/libvirt/-/work_items/913 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Fri, Aug 21, 2026 at 12:51:37PM +0200, Ján Tomko via Devel wrote:
From: Ján Tomko <jtomko@redhat.com>
If parsed number of extents is 0, the calculated buffer size won't hold the first regex_unit.
Closes: https://gitlab.com/libvirt/libvirt/-/work_items/913 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 51e9337820..81f9d62d8b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -161,12 +161,12 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDef *vol,
/* Allocate space for 'nextents' regex_unit strings plus a comma for each */ regex = g_new0(char, nextents * (strlen(regex_unit) + 1) + 1); - strcat(regex, regex_unit); - for (i = 1; i < nextents; i++) { + for (i = 0; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); strcat(regex, regex_unit); + strcat(regex, ","); } + regex[strlen(regex) - 1] = '\0';
We allocated 'regex' with 'g_new0', with 1 more byte than we need, so it is always going to be NUL terminated. This writes to regex[-1] when nextents == 0, so is both redundant and causing a bug. Drop the line and we're fine. As a more general point, IMHO any code which involves using g_new with a string length calculation ought to be killed and replaced with GString so we eliminate the entire class of errors with string bounds access. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
On a Thursday in 2026, Daniel P. Berrangé wrote:
On Fri, Aug 21, 2026 at 12:51:37PM +0200, Ján Tomko via Devel wrote:
From: Ján Tomko <jtomko@redhat.com>
If parsed number of extents is 0, the calculated buffer size won't hold the first regex_unit.
Closes: https://gitlab.com/libvirt/libvirt/-/work_items/913 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 51e9337820..81f9d62d8b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -161,12 +161,12 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDef *vol,
/* Allocate space for 'nextents' regex_unit strings plus a comma for each */ regex = g_new0(char, nextents * (strlen(regex_unit) + 1) + 1); - strcat(regex, regex_unit); - for (i = 1; i < nextents; i++) { + for (i = 0; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); strcat(regex, regex_unit); + strcat(regex, ","); } + regex[strlen(regex) - 1] = '\0';
We allocated 'regex' with 'g_new0', with 1 more byte than we need, so it is always going to be NUL terminated.
The intention was to erase the trailing comma, similar to virBufferTrim(buf, ",');
This writes to regex[-1] when nextents == 0, so is both redundant and causing a bug. Drop the line and we're fine.
Sorry for not self-NACKing earlier, it would have spared you from writing this reply. Jano
As a more general point, IMHO any code which involves using g_new with a string length calculation ought to be killed and replaced with GString so we eliminate the entire class of errors with string bounds access.
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník