| Message ID | 20240919150611.17074-1-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration/multifd: Ensure packet->ramblock is null-terminated | expand |
On Thu, Sep 19, 2024 at 12:06:11PM -0300, Fabiano Rosas wrote: > Coverity points out that the current usage of strncpy to write the > ramblock name allows the field to not have an ending '\0' in case > idstr is already not null-terminated (e.g. if it's larger than 256 > bytes). > > This is currently harmless because the packet->ramblock field is never > touched again on the source side. The destination side reads only up > to the field's size from the stream and forces the last byte to be 0. > > We're still open to a programming error in the future in case this > field is ever passed into a function that expects a null-terminated > string. > > Change from strncpy to QEMU's pstrcpy, which puts a '\0' at the end of > the string and doesn't fill the extra space with zeros. > > (there's no spillage between iterations of fill_packet because after > commit 87bb9e953e ("migration/multifd: Isolate ram pages packet data") > the packet is always zeroed before filling) > > Resolves: Coverity CID 1560071 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Fabiano Rosas <farosas@suse.de> queued.
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c index 07c63f4a72..55191152f9 100644 --- a/migration/multifd-nocomp.c +++ b/migration/multifd-nocomp.c @@ -17,6 +17,7 @@ #include "multifd.h" #include "options.h" #include "qapi/error.h" +#include "qemu/cutils.h" #include "qemu/error-report.h" #include "trace.h" @@ -201,7 +202,8 @@ void multifd_ram_fill_packet(MultiFDSendParams *p) packet->zero_pages = cpu_to_be32(zero_num); if (pages->block) { - strncpy(packet->ramblock, pages->block->idstr, 256); + pstrcpy(packet->ramblock, sizeof(packet->ramblock), + pages->block->idstr); } for (int i = 0; i < pages->num; i++) {
Coverity points out that the current usage of strncpy to write the ramblock name allows the field to not have an ending '\0' in case idstr is already not null-terminated (e.g. if it's larger than 256 bytes). This is currently harmless because the packet->ramblock field is never touched again on the source side. The destination side reads only up to the field's size from the stream and forces the last byte to be 0. We're still open to a programming error in the future in case this field is ever passed into a function that expects a null-terminated string. Change from strncpy to QEMU's pstrcpy, which puts a '\0' at the end of the string and doesn't fill the extra space with zeros. (there's no spillage between iterations of fill_packet because after commit 87bb9e953e ("migration/multifd: Isolate ram pages packet data") the packet is always zeroed before filling) Resolves: Coverity CID 1560071 Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/multifd-nocomp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)