| Message ID | 5F9975F1.4080205@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] plugins: Fix resource leak in connect_socket() | expand |
Kindly ping. On 2020/10/28 21:45, AlexChen wrote: > Either accept() fails or exits normally, we need to close the fd. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: AlexChen <alex.chen@huawei.com> > --- > contrib/plugins/lockstep.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c > index 319bd44b83..5aad50869d 100644 > --- a/contrib/plugins/lockstep.c > +++ b/contrib/plugins/lockstep.c > @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) > socket_fd = accept(fd, NULL, NULL); > if (socket_fd < 0 && errno != EINTR) { > perror("accept socket"); > + close(fd); > return false; > } > > qemu_plugin_outs("setup_socket::ready\n"); > > + close(fd); > return true; > } >
On 28/10/2020 14.45, AlexChen wrote: > Either accept() fails or exits normally, we need to close the fd. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: AlexChen <alex.chen@huawei.com> > --- > contrib/plugins/lockstep.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c > index 319bd44b83..5aad50869d 100644 > --- a/contrib/plugins/lockstep.c > +++ b/contrib/plugins/lockstep.c > @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) > socket_fd = accept(fd, NULL, NULL); I think you could also simply close(fd) here instead, then you don't have to do it twice below. Thomas > if (socket_fd < 0 && errno != EINTR) { > perror("accept socket"); > + close(fd); > return false; > } > > qemu_plugin_outs("setup_socket::ready\n"); > > + close(fd); > return true; > } >
On 2020/11/17 0:50, Thomas Huth wrote: > On 28/10/2020 14.45, AlexChen wrote: >> Either accept() fails or exits normally, we need to close the fd. >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: AlexChen <alex.chen@huawei.com> >> --- >> contrib/plugins/lockstep.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c >> index 319bd44b83..5aad50869d 100644 >> --- a/contrib/plugins/lockstep.c >> +++ b/contrib/plugins/lockstep.c >> @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) >> socket_fd = accept(fd, NULL, NULL); > > I think you could also simply close(fd) here instead, then you don't have to > do it twice below. > Hi Thomas and Alex, Thanks for your suggestion. It's a simple and effective solution. Considering that the patch v3 has been queued by Alex Bennée, May I modify this patch and then send patch v4? Thanks, Alex > >> if (socket_fd < 0 && errno != EINTR) { >> perror("accept socket"); >> + close(fd); >> return false; >> } >> >> qemu_plugin_outs("setup_socket::ready\n"); >> >> + close(fd); >> return true; >> } >> > > . >
Alex Chen <alex.chen@huawei.com> writes: > On 2020/11/17 0:50, Thomas Huth wrote: >> On 28/10/2020 14.45, AlexChen wrote: >>> Either accept() fails or exits normally, we need to close the fd. >>> >>> Reported-by: Euler Robot <euler.robot@huawei.com> >>> Signed-off-by: AlexChen <alex.chen@huawei.com> >>> --- >>> contrib/plugins/lockstep.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c >>> index 319bd44b83..5aad50869d 100644 >>> --- a/contrib/plugins/lockstep.c >>> +++ b/contrib/plugins/lockstep.c >>> @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) >>> socket_fd = accept(fd, NULL, NULL); >> >> I think you could also simply close(fd) here instead, then you don't have to >> do it twice below. >> > > Hi Thomas and Alex, > Thanks for your suggestion. It's a simple and effective solution. > Considering that the patch v3 has been queued by Alex Bennée, > May I modify this patch and then send patch v4? The fix has already been merged so a fresh patch would make more sense.
On 17/11/2020 12.35, Alex Bennée wrote: > > Alex Chen <alex.chen@huawei.com> writes: > >> On 2020/11/17 0:50, Thomas Huth wrote: >>> On 28/10/2020 14.45, AlexChen wrote: >>>> Either accept() fails or exits normally, we need to close the fd. >>>> >>>> Reported-by: Euler Robot <euler.robot@huawei.com> >>>> Signed-off-by: AlexChen <alex.chen@huawei.com> >>>> --- >>>> contrib/plugins/lockstep.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c >>>> index 319bd44b83..5aad50869d 100644 >>>> --- a/contrib/plugins/lockstep.c >>>> +++ b/contrib/plugins/lockstep.c >>>> @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) >>>> socket_fd = accept(fd, NULL, NULL); >>> >>> I think you could also simply close(fd) here instead, then you don't have to >>> do it twice below. >>> >> >> Hi Thomas and Alex, >> Thanks for your suggestion. It's a simple and effective solution. >> Considering that the patch v3 has been queued by Alex Bennée, >> May I modify this patch and then send patch v4? > > The fix has already been merged so a fresh patch would make more sense. Well, then never mind. It's not worth the code churn to just get rid of one line of code, I think. Thomas
diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c index 319bd44b83..5aad50869d 100644 --- a/contrib/plugins/lockstep.c +++ b/contrib/plugins/lockstep.c @@ -268,11 +268,13 @@ static bool setup_socket(const char *path) socket_fd = accept(fd, NULL, NULL); if (socket_fd < 0 && errno != EINTR) { perror("accept socket"); + close(fd); return false; } qemu_plugin_outs("setup_socket::ready\n"); + close(fd); return true; }
Either accept() fails or exits normally, we need to close the fd. Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: AlexChen <alex.chen@huawei.com> --- contrib/plugins/lockstep.c | 2 ++ 1 file changed, 2 insertions(+)