From: Michal Privoznik <mprivozn@redhat.com>

Inside of virPCIDeviceReadClass() there's a call to
virFileReadAll(). This reads contents of given file into a buffer
(id_str). To make sure the buffer is NUL terminated string
there's then write of NUL byte at 9th position of the buffer.
Well, this is redundant as virFileReadAll() made sure the buffer
is properly terminated on success (transitively¸ via
saferead_lim()). But it is also wrong, because there's no
guarantee the file is more than 8 bytes long.

Just remove the NUL termination and rely on virFileReadAll() to
properly terminate the buffer.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virpci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index ca6f2e8210..2e32ed17ff 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -487,7 +487,6 @@ virPCIDeviceReadClass(virPCIDevice *dev, uint16_t *device_class)
     if (virFileReadAll(path, 9, &id_str) < 0)
         return -1;
 
-    id_str[8] = '\0';
     if (virStrToLong_ui(id_str, NULL, 16, &value) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Unusual value in %1$s/devices/%2$s/class: %3$s"),
-- 
2.52.0

On Fri, Mar 27, 2026 at 10:13:26 +0100, Michal Privoznik via Devel wrote:
> From: Michal Privoznik <mprivozn@redhat.com>
> 
> Inside of virPCIDeviceReadClass() there's a call to
> virFileReadAll(). This reads contents of given file into a buffer
> (id_str). To make sure the buffer is NUL terminated string
> there's then write of NUL byte at 9th position of the buffer.
> Well, this is redundant as virFileReadAll() made sure the buffer
> is properly terminated on success (transitively¸ via
> saferead_lim()). But it is also wrong, because there's no
> guarantee the file is more than 8 bytes long.

I've just posted an more extensive series, which also documents
virFileReadAll to prevent such misunderstandings, fixes one more
instance of the same problem which didn't yet manifest itself because
the overwritten byte was exactly the NUL terminator, and removes few
other redundant attempts to NUL-out the end of the buffer from
virFileReadAll and friends.

> Just remove the NUL termination and rely on virFileReadAll() to
> properly terminate the buffer.

If you want to push this now you can:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On 3/27/26 10:57, Peter Krempa wrote:
> On Fri, Mar 27, 2026 at 10:13:26 +0100, Michal Privoznik via Devel wrote:
>> From: Michal Privoznik <mprivozn@redhat.com>
>>
>> Inside of virPCIDeviceReadClass() there's a call to
>> virFileReadAll(). This reads contents of given file into a buffer
>> (id_str). To make sure the buffer is NUL terminated string
>> there's then write of NUL byte at 9th position of the buffer.
>> Well, this is redundant as virFileReadAll() made sure the buffer
>> is properly terminated on success (transitively¸ via
>> saferead_lim()). But it is also wrong, because there's no
>> guarantee the file is more than 8 bytes long.
> 
> I've just posted an more extensive series, which also documents
> virFileReadAll to prevent such misunderstandings, fixes one more
> instance of the same problem which didn't yet manifest itself because
> the overwritten byte was exactly the NUL terminator, and removes few
> other redundant attempts to NUL-out the end of the buffer from
> virFileReadAll and friends.
> 
>> Just remove the NUL termination and rely on virFileReadAll() to
>> properly terminate the buffer.
> 
> If you want to push this now you can:
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

Nah, yours is more extensive so lets go with your patches.

Michal