| Message ID | 20240829131005.9196-1-gaoshiyuan@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] platform-bus: fix refcount leak | expand |
> -----Original Message----- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org > <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of Gao > Shiyuan via > Sent: Thursday, August 29, 2024 9:10 PM > To: Paolo Bonzini <pbonzini@redhat.com> > Cc: qemu-devel@nongnu.org; gaoshiyuan@baidu.com > Subject: [PATCH 1/1] platform-bus: fix refcount leak > > Temporary object causes reference count leakage. > > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> > --- > hw/core/platform-bus.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c > index b8487b26b6..dc58bf505a 100644 > --- a/hw/core/platform-bus.c > +++ b/hw/core/platform-bus.c > @@ -145,9 +145,12 @@ static void platform_bus_map_mmio(PlatformBusDevice > *pbus, SysBusDevice *sbdev, > * the target device's memory region > */ > for (off = 0; off < pbus->mmio_size; off += alignment) { > - if (!memory_region_find(&pbus->mmio, off, size).mr) { > + MemoryRegion *mr = memory_region_find(&pbus->mmio, off, size).mr; > + if (!mr) { > found_region = true; > break; > + } else { > + memory_region_unref(mr); > } LGTM, but if the empty region is not found, the process will stop running, so I think this bug may be not serious. > } > > -- > 2.39.3 (Apple Git-146) >
On Fri, 30 Aug 2024 at 11:47, Xingtao Yao (Fujitsu) via <qemu-devel@nongnu.org> wrote: > > > > > -----Original Message----- > > From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org > > <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of Gao > > Shiyuan via > > Sent: Thursday, August 29, 2024 9:10 PM > > To: Paolo Bonzini <pbonzini@redhat.com> > > Cc: qemu-devel@nongnu.org; gaoshiyuan@baidu.com > > Subject: [PATCH 1/1] platform-bus: fix refcount leak > > > > Temporary object causes reference count leakage. > > > > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> > > --- > > hw/core/platform-bus.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c > > index b8487b26b6..dc58bf505a 100644 > > --- a/hw/core/platform-bus.c > > +++ b/hw/core/platform-bus.c > > @@ -145,9 +145,12 @@ static void platform_bus_map_mmio(PlatformBusDevice > > *pbus, SysBusDevice *sbdev, > > * the target device's memory region > > */ > > for (off = 0; off < pbus->mmio_size; off += alignment) { > > - if (!memory_region_find(&pbus->mmio, off, size).mr) { > > + MemoryRegion *mr = memory_region_find(&pbus->mmio, off, size).mr; > > + if (!mr) { > > found_region = true; > > break; > > + } else { > > + memory_region_unref(mr); > > } > LGTM, but if the empty region is not found, the process will stop running, so I think this bug may be not > serious. It's not a major leak, but in the case where the region we're scanning already has some MRs at the start followed by a big enough empty region, we will first find and leak those initial MRs before we find and use the empty space. So there are some usage patterns where we'll leak something and not immediately exit QEMU. Applied to target-arm.next, thanks (with the commit message tweaked). -- PMM
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c index b8487b26b6..dc58bf505a 100644 --- a/hw/core/platform-bus.c +++ b/hw/core/platform-bus.c @@ -145,9 +145,12 @@ static void platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev, * the target device's memory region */ for (off = 0; off < pbus->mmio_size; off += alignment) { - if (!memory_region_find(&pbus->mmio, off, size).mr) { + MemoryRegion *mr = memory_region_find(&pbus->mmio, off, size).mr; + if (!mr) { found_region = true; break; + } else { + memory_region_unref(mr); } }
Temporary object causes reference count leakage. Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com> --- hw/core/platform-bus.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)