On Mon, 3 Nov 2025 at 12:05, John Levon <[email protected]> wrote: > > On Mon, Nov 03, 2025 at 11:34:36AM +0000, Peter Maydell wrote: > > > > > > Hi; Coverity suggests there are some issues with this code > > > > > (CID 1611807, 1611808, 1611809): > > > > > > > > Hi; it looks like 1611807 and 1611808 (the resource leaks) > > > > are still present in this code in current git; would somebody > > > > like to have a look at this? > > > > > > Please see > > > https://lore.kernel.org/qemu-devel/[email protected]/ > > > > > > I believe them to be false positives. > > > > That email only seems to talk about the locking issue (1611806, > > 1611809) which we've marked as false-positives in Coverity. > > But 1611807 and 1611808 are "failed to free resources" issues: > > we allocate memory into reqfds and msg, but in the error > > exit path (e.g. if we "goto fatal" because qio_channel_read() > > failed) it doesn't look like we ever free that memory. > > Sorry, I should have pasted: Here was my reply to Cédric below. > > Looking at it with fresh eyes, though, I'm not so sure, at least in the case > we > alloc instead of freelist, we need some form of recycle on the error path. The > whole function needs a big refactor for clarity... > > > *** CID 1611808: Resource leaks (RESOURCE_LEAK) > > /builds/qemu-project/qemu/hw/vfio-user/proxy.c: 411 in > > vfio_user_recv_one() > > 405 if (isreply && msg != NULL) { > > 406 /* force an error to keep sending thread from hanging */ > > 407 vfio_user_set_error(msg->hdr, EINVAL); > > 408 msg->complete = true; > > 409 qemu_cond_signal(&msg->cv); > > 410 } > > > > > CID 1611808: Resource leaks (RESOURCE_LEAK) > > > > > Variable "reqfds" going out of scope leaks the storage it points > > > > > to. > > 411 return -1; > > vfio_user_getmsg() I think takes ownership of reqfds so this looks OK.
The error reported in CID 1611808 is that if we take the "goto err" code path for "vfio_user_recv request larger than max" then we never call vfio_user_getmsg() to take ownership of reqfds, but the 'err' codepath does not free reqfds. > It's hard to say without the full analysis. If you like you can ask for an account at https://scan.coverity.com/projects/qemu which will let you look at the issues in the web GUI, which is a bit more detailed than the email summaries. > > /builds/qemu-project/qemu/hw/vfio-user/proxy.c: 411 in > > vfio_user_recv_one() > > 405 if (isreply && msg != NULL) { > > 406 /* force an error to keep sending thread from hanging */ > > 407 vfio_user_set_error(msg->hdr, EINVAL); > > 408 msg->complete = true; > > 409 qemu_cond_signal(&msg->cv); > > 410 } > > > > > CID 1611807: Resource leaks (RESOURCE_LEAK) > > > > > Variable "msg" going out of scope leaks the storage it points to. > > 411 return -1; > > 412 } > > Same as above. In 1611807 the error path is slightly different: here we do call vfio_user_getmsg(), which allocates 'msg' and returns it to us. Then we get a failure return from qio_channel_read() which causes us to take a "goto fatal", so we never call vfio_user_process() to take ownership of 'msg'; but we don't free it or otherwise dispose of it in the 'fatal' codepath. I think it might also make the code more readable if it was refactored to keep the is_reply and !is_reply codepaths separate. At the moment the error handling is complicated because the second half of the function has two consecutive "if (is_reply) { ... } else { ... }" long code blocks, and the error handling depends on whether we're is_reply or not. thanks -- PMM
