* [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 10:16 ` Alex Chen 0 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 10:16 UTC (permalink / raw) To: groug, qemu_oss; +Cc: alex.chen, qemu-devel, qemu-trivial, zhang.zhanghailiang Only one of the options -s and -f can be used. When -f is used, the fd is created externally and does not need to be closed. When -s is used, a new socket fd is created, and this socket fd needs to be closed at the end of main(). Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Alex Chen <alex.chen@huawei.com> --- fsdev/virtfs-proxy-helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 15c0e79b06..339d477169 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) process_requests(sock); error: g_free(rpath); + if (sock_name) { + close(sock); + } g_free(sock_name); do_log(LOG_INFO, "Done\n"); closelog(); -- 2.19.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 10:16 ` Alex Chen 0 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 10:16 UTC (permalink / raw) To: groug, qemu_oss; +Cc: alex.chen, qemu-trivial, qemu-devel, zhang.zhanghailiang Only one of the options -s and -f can be used. When -f is used, the fd is created externally and does not need to be closed. When -s is used, a new socket fd is created, and this socket fd needs to be closed at the end of main(). Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Alex Chen <alex.chen@huawei.com> --- fsdev/virtfs-proxy-helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 15c0e79b06..339d477169 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) process_requests(sock); error: g_free(rpath); + if (sock_name) { + close(sock); + } g_free(sock_name); do_log(LOG_INFO, "Done\n"); closelog(); -- 2.19.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 10:16 ` Alex Chen @ 2020-11-26 10:50 ` Li Qiang -1 siblings, 0 replies; 18+ messages in thread From: Li Qiang @ 2020-11-26 10:50 UTC (permalink / raw) To: Alex Chen Cc: Greg Kurz, qemu_oss, qemu-trivial, Qemu Developers, zhanghailiang Alex Chen <alex.chen@huawei.com> 于2020年11月26日周四 下午6:29写道： > > Only one of the options -s and -f can be used. When -f is used, > the fd is created externally and does not need to be closed. > When -s is used, a new socket fd is created, and this socket fd > needs to be closed at the end of main(). > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > fsdev/virtfs-proxy-helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..339d477169 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name) { > + close(sock); > + } If 'proxy_socket' failed, you call close(-1). Maybe following is better? if (sock >= 0) { close(sock); } Thanks, Li Qiang > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); > -- > 2.19.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 10:50 ` Li Qiang 0 siblings, 0 replies; 18+ messages in thread From: Li Qiang @ 2020-11-26 10:50 UTC (permalink / raw) To: Alex Chen Cc: qemu-trivial, zhanghailiang, qemu_oss, Greg Kurz, Qemu Developers Alex Chen <alex.chen@huawei.com> 于2020年11月26日周四 下午6:29写道： > > Only one of the options -s and -f can be used. When -f is used, > the fd is created externally and does not need to be closed. > When -s is used, a new socket fd is created, and this socket fd > needs to be closed at the end of main(). > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > fsdev/virtfs-proxy-helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..339d477169 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name) { > + close(sock); > + } If 'proxy_socket' failed, you call close(-1). Maybe following is better? if (sock >= 0) { close(sock); } Thanks, Li Qiang > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); > -- > 2.19.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 10:50 ` Li Qiang @ 2020-11-26 11:40 ` Alex Chen -1 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 11:40 UTC (permalink / raw) To: Li Qiang Cc: Greg Kurz, qemu_oss, qemu-trivial, Qemu Developers, zhanghailiang On 2020/11/26 18:50, Li Qiang wrote: > Alex Chen <alex.chen@huawei.com> >> >> Only one of the options -s and -f can be used. When -f is used, >> the fd is created externally and does not need to be closed. >> When -s is used, a new socket fd is created, and this socket fd >> needs to be closed at the end of main(). >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> --- >> fsdev/virtfs-proxy-helper.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c >> index 15c0e79b06..339d477169 100644 >> --- a/fsdev/virtfs-proxy-helper.c >> +++ b/fsdev/virtfs-proxy-helper.c >> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) >> process_requests(sock); >> error: >> g_free(rpath); >> + if (sock_name) { >> + close(sock); >> + } > > If 'proxy_socket' failed, you call close(-1). > > Maybe following is better? > > if (sock >= 0) { > close(sock); > } > Hi Qiang, Thanks for your review. The 'sock' need to be closed only when option -s is used, that is when 'sock_name' is not NULL. So maybe the following is better? diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 15c0e79b06..3ba68d9878 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) process_requests(sock); error: g_free(rpath); + if (sock_name && (sock >= 0)) { + close(sock); + } g_free(sock_name); do_log(LOG_INFO, "Done\n"); closelog(); Thanks, Alex ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 11:40 ` Alex Chen 0 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 11:40 UTC (permalink / raw) To: Li Qiang Cc: qemu-trivial, zhanghailiang, qemu_oss, Greg Kurz, Qemu Developers On 2020/11/26 18:50, Li Qiang wrote: > Alex Chen <alex.chen@huawei.com> >> >> Only one of the options -s and -f can be used. When -f is used, >> the fd is created externally and does not need to be closed. >> When -s is used, a new socket fd is created, and this socket fd >> needs to be closed at the end of main(). >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> --- >> fsdev/virtfs-proxy-helper.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c >> index 15c0e79b06..339d477169 100644 >> --- a/fsdev/virtfs-proxy-helper.c >> +++ b/fsdev/virtfs-proxy-helper.c >> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) >> process_requests(sock); >> error: >> g_free(rpath); >> + if (sock_name) { >> + close(sock); >> + } > > If 'proxy_socket' failed, you call close(-1). > > Maybe following is better? > > if (sock >= 0) { > close(sock); > } > Hi Qiang, Thanks for your review. The 'sock' need to be closed only when option -s is used, that is when 'sock_name' is not NULL. So maybe the following is better? diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 15c0e79b06..3ba68d9878 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) process_requests(sock); error: g_free(rpath); + if (sock_name && (sock >= 0)) { + close(sock); + } g_free(sock_name); do_log(LOG_INFO, "Done\n"); closelog(); Thanks, Alex ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 11:40 ` Alex Chen @ 2020-11-26 15:04 ` Li Qiang -1 siblings, 0 replies; 18+ messages in thread From: Li Qiang @ 2020-11-26 15:04 UTC (permalink / raw) To: Alex Chen Cc: Greg Kurz, qemu_oss, qemu-trivial, Qemu Developers, zhanghailiang Alex Chen <alex.chen@huawei.com> 于2020年11月26日周四 下午7:40写道： > > On 2020/11/26 18:50, Li Qiang wrote: > > Alex Chen <alex.chen@huawei.com> > >> > >> Only one of the options -s and -f can be used. When -f is used, > >> the fd is created externally and does not need to be closed. > >> When -s is used, a new socket fd is created, and this socket fd > >> needs to be closed at the end of main(). > >> > >> Reported-by: Euler Robot <euler.robot@huawei.com> > >> Signed-off-by: Alex Chen <alex.chen@huawei.com> > >> --- > >> fsdev/virtfs-proxy-helper.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > >> index 15c0e79b06..339d477169 100644 > >> --- a/fsdev/virtfs-proxy-helper.c > >> +++ b/fsdev/virtfs-proxy-helper.c > >> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > >> process_requests(sock); > >> error: > >> g_free(rpath); > >> + if (sock_name) { > >> + close(sock); > >> + } > > > > If 'proxy_socket' failed, you call close(-1). > > > > Maybe following is better? > > > > if (sock >= 0) { > > close(sock); > > } > > > > Hi Qiang, > > Thanks for your review. > The 'sock' need to be closed only when option -s is used, that is when 'sock_name' is not NULL. > So maybe the following is better? Yes, you're right. > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..3ba68d9878 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name && (sock >= 0)) { No need parenthesis for 'sock>=0'? Thanks, Li Qiang > + close(sock); > + } > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); > > Thanks, > Alex > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 15:04 ` Li Qiang 0 siblings, 0 replies; 18+ messages in thread From: Li Qiang @ 2020-11-26 15:04 UTC (permalink / raw) To: Alex Chen Cc: qemu-trivial, zhanghailiang, qemu_oss, Greg Kurz, Qemu Developers Alex Chen <alex.chen@huawei.com> 于2020年11月26日周四 下午7:40写道： > > On 2020/11/26 18:50, Li Qiang wrote: > > Alex Chen <alex.chen@huawei.com> > >> > >> Only one of the options -s and -f can be used. When -f is used, > >> the fd is created externally and does not need to be closed. > >> When -s is used, a new socket fd is created, and this socket fd > >> needs to be closed at the end of main(). > >> > >> Reported-by: Euler Robot <euler.robot@huawei.com> > >> Signed-off-by: Alex Chen <alex.chen@huawei.com> > >> --- > >> fsdev/virtfs-proxy-helper.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > >> index 15c0e79b06..339d477169 100644 > >> --- a/fsdev/virtfs-proxy-helper.c > >> +++ b/fsdev/virtfs-proxy-helper.c > >> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > >> process_requests(sock); > >> error: > >> g_free(rpath); > >> + if (sock_name) { > >> + close(sock); > >> + } > > > > If 'proxy_socket' failed, you call close(-1). > > > > Maybe following is better? > > > > if (sock >= 0) { > > close(sock); > > } > > > > Hi Qiang, > > Thanks for your review. > The 'sock' need to be closed only when option -s is used, that is when 'sock_name' is not NULL. > So maybe the following is better? Yes, you're right. > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..3ba68d9878 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name && (sock >= 0)) { No need parenthesis for 'sock>=0'? Thanks, Li Qiang > + close(sock); > + } > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); > > Thanks, > Alex > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 10:16 ` Alex Chen @ 2020-11-26 12:07 ` Greg Kurz -1 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-26 12:07 UTC (permalink / raw) To: Alex Chen; +Cc: qemu_oss, qemu-devel, qemu-trivial, zhang.zhanghailiang On Thu, 26 Nov 2020 10:16:24 +0000 Alex Chen <alex.chen@huawei.com> wrote: > Only one of the options -s and -f can be used. When -f is used, > the fd is created externally and does not need to be closed. The process running virtfs-proxy-helper has its own copy of the fd inherited from its parent. And this fd will be closed eventually when the process terminates. > When -s is used, a new socket fd is created, and this socket fd > needs to be closed at the end of main(). > Same here, the new socket fd is closed when the process terminates. The only justification to merge such a change would be if the code was sitting in some other function, in which case we should indeed do proper rollback. But it is main() here, so this patch isn't needed. > Reported-by: Euler Robot <euler.robot@huawei.com> Can you provide a copy of the report in case I'm missing something ? > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > fsdev/virtfs-proxy-helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..339d477169 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name) { > + close(sock); > + } > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 12:07 ` Greg Kurz 0 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-26 12:07 UTC (permalink / raw) To: Alex Chen; +Cc: qemu-trivial, qemu_oss, qemu-devel, zhang.zhanghailiang On Thu, 26 Nov 2020 10:16:24 +0000 Alex Chen <alex.chen@huawei.com> wrote: > Only one of the options -s and -f can be used. When -f is used, > the fd is created externally and does not need to be closed. The process running virtfs-proxy-helper has its own copy of the fd inherited from its parent. And this fd will be closed eventually when the process terminates. > When -s is used, a new socket fd is created, and this socket fd > needs to be closed at the end of main(). > Same here, the new socket fd is closed when the process terminates. The only justification to merge such a change would be if the code was sitting in some other function, in which case we should indeed do proper rollback. But it is main() here, so this patch isn't needed. > Reported-by: Euler Robot <euler.robot@huawei.com> Can you provide a copy of the report in case I'm missing something ? > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > fsdev/virtfs-proxy-helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 15c0e79b06..339d477169 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -1154,6 +1154,9 @@ int main(int argc, char **argv) > process_requests(sock); > error: > g_free(rpath); > + if (sock_name) { > + close(sock); > + } > g_free(sock_name); > do_log(LOG_INFO, "Done\n"); > closelog(); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 12:07 ` Greg Kurz @ 2020-11-26 13:15 ` Alex Chen -1 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 13:15 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu_oss, qemu-devel, qemu-trivial, zhang.zhanghailiang Hi Greg, Thanks for your review. On 2020/11/26 20:07, Greg Kurz wrote: > On Thu, 26 Nov 2020 10:16:24 +0000 > Alex Chen <alex.chen@huawei.com> wrote: > >> Only one of the options -s and -f can be used. When -f is used, >> the fd is created externally and does not need to be closed. > > The process running virtfs-proxy-helper has its own copy of > the fd inherited from its parent. And this fd will be closed > eventually when the process terminates. > >> When -s is used, a new socket fd is created, and this socket fd >> needs to be closed at the end of main(). >> > > Same here, the new socket fd is closed when the process > terminates. IMO, it's best to explicitly release resources before the process terminates, just as the variable 'rpath' is explicitly freed in main(), so socket fd also needs to be explicitly closed here. Looking forward to your reply. > > The only justification to merge such a change would be if > the code was sitting in some other function, in which > case we should indeed do proper rollback. But it is main() > here, so this patch isn't needed. > >> Reported-by: Euler Robot <euler.robot@huawei.com> > > Can you provide a copy of the report in case I'm > missing something ? > Our codecheck tool reports a resource leak here, which is relatively simple, like the one below, I did not attach it. --------------------- "Resource leak: sock" --------------------- Thanks, Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 13:15 ` Alex Chen 0 siblings, 0 replies; 18+ messages in thread From: Alex Chen @ 2020-11-26 13:15 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-trivial, qemu_oss, qemu-devel, zhang.zhanghailiang Hi Greg, Thanks for your review. On 2020/11/26 20:07, Greg Kurz wrote: > On Thu, 26 Nov 2020 10:16:24 +0000 > Alex Chen <alex.chen@huawei.com> wrote: > >> Only one of the options -s and -f can be used. When -f is used, >> the fd is created externally and does not need to be closed. > > The process running virtfs-proxy-helper has its own copy of > the fd inherited from its parent. And this fd will be closed > eventually when the process terminates. > >> When -s is used, a new socket fd is created, and this socket fd >> needs to be closed at the end of main(). >> > > Same here, the new socket fd is closed when the process > terminates. IMO, it's best to explicitly release resources before the process terminates, just as the variable 'rpath' is explicitly freed in main(), so socket fd also needs to be explicitly closed here. Looking forward to your reply. > > The only justification to merge such a change would be if > the code was sitting in some other function, in which > case we should indeed do proper rollback. But it is main() > here, so this patch isn't needed. > >> Reported-by: Euler Robot <euler.robot@huawei.com> > > Can you provide a copy of the report in case I'm > missing something ? > Our codecheck tool reports a resource leak here, which is relatively simple, like the one below, I did not attach it. --------------------- "Resource leak: sock" --------------------- Thanks, Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 13:15 ` Alex Chen (?) @ 2020-11-26 17:52 ` Christian Schoenebeck 2020-11-26 18:27 ` Greg Kurz -1 siblings, 1 reply; 18+ messages in thread From: Christian Schoenebeck @ 2020-11-26 17:52 UTC (permalink / raw) To: qemu-devel; +Cc: Alex Chen, Greg Kurz, qemu-trivial, zhang.zhanghailiang On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote: > Hi Greg, > > Thanks for your review. > > On 2020/11/26 20:07, Greg Kurz wrote: > > On Thu, 26 Nov 2020 10:16:24 +0000 > > > > Alex Chen <alex.chen@huawei.com> wrote: > >> Only one of the options -s and -f can be used. When -f is used, > >> the fd is created externally and does not need to be closed. So somebody is really using the 9p proxy driver for something; interesting. > > > > The process running virtfs-proxy-helper has its own copy of > > the fd inherited from its parent. And this fd will be closed > > eventually when the process terminates. > > > >> When -s is used, a new socket fd is created, and this socket fd > >> needs to be closed at the end of main(). > > > > Same here, the new socket fd is closed when the process > > terminates. Does it? I haven't reviewed much of the 9p proxy code yet, however if chroot() fails for instance, the fd would leak right now, wouldn't it? Or was your argument that it's the OS's job to free any file descriptor automatically on process terminations in general? > IMO, it's best to explicitly release resources before the process > terminates, just as the variable 'rpath' is explicitly freed in main(), > so socket fd also needs to be explicitly closed here. > > Looking forward to your reply. > > > The only justification to merge such a change would be if > > the code was sitting in some other function, in which > > case we should indeed do proper rollback. But it is main() > > here, so this patch isn't needed. > > > >> Reported-by: Euler Robot <euler.robot@huawei.com> > > > > Can you provide a copy of the report in case I'm > > missing something ? > > Our codecheck tool reports a resource leak here, which is relatively simple, > like the one below, I did not attach it. > > --------------------- > "Resource leak: sock" > --------------------- Yeah, not very helpful that output. > > Thanks, > Alex Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 17:52 ` Christian Schoenebeck @ 2020-11-26 18:27 ` Greg Kurz 0 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-26 18:27 UTC (permalink / raw) To: Christian Schoenebeck Cc: qemu-devel, Alex Chen, qemu-trivial, zhang.zhanghailiang On Thu, 26 Nov 2020 18:52:39 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote: > > Hi Greg, > > > > Thanks for your review. > > > > On 2020/11/26 20:07, Greg Kurz wrote: > > > On Thu, 26 Nov 2020 10:16:24 +0000 > > > > > > Alex Chen <alex.chen@huawei.com> wrote: > > >> Only one of the options -s and -f can be used. When -f is used, > > >> the fd is created externally and does not need to be closed. > > So somebody is really using the 9p proxy driver for something; interesting. > > > > > > > The process running virtfs-proxy-helper has its own copy of > > > the fd inherited from its parent. And this fd will be closed > > > eventually when the process terminates. > > > > > >> When -s is used, a new socket fd is created, and this socket fd > > >> needs to be closed at the end of main(). > > > > > > Same here, the new socket fd is closed when the process > > > terminates. > > Does it? I haven't reviewed much of the 9p proxy code yet, however if chroot() > fails for instance, the fd would leak right now, wouldn't it? > This is done just at the end of main()... the leak won't last long. > Or was your argument that it's the OS's job to free any file descriptor > automatically on process terminations in general? > That's exactly my point. The only justification that'd deserve to be in the changelog of such a patch is something like "because this is good practice to rollback in case code moves to another function than main()". > > IMO, it's best to explicitly release resources before the process > > terminates, just as the variable 'rpath' is explicitly freed in main(), > > so socket fd also needs to be explicitly closed here. > > > > Looking forward to your reply. > > > > > The only justification to merge such a change would be if > > > the code was sitting in some other function, in which > > > case we should indeed do proper rollback. But it is main() > > > here, so this patch isn't needed. > > > > > >> Reported-by: Euler Robot <euler.robot@huawei.com> > > > > > > Can you provide a copy of the report in case I'm > > > missing something ? > > > > Our codecheck tool reports a resource leak here, which is relatively simple, > > like the one below, I did not attach it. > > > > --------------------- > > "Resource leak: sock" > > --------------------- > > Yeah, not very helpful that output. > Indeed :D > > > > Thanks, > > Alex > > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-26 18:27 ` Greg Kurz 0 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-26 18:27 UTC (permalink / raw) To: Christian Schoenebeck Cc: Alex Chen, qemu-trivial, qemu-devel, zhang.zhanghailiang On Thu, 26 Nov 2020 18:52:39 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote: > > Hi Greg, > > > > Thanks for your review. > > > > On 2020/11/26 20:07, Greg Kurz wrote: > > > On Thu, 26 Nov 2020 10:16:24 +0000 > > > > > > Alex Chen <alex.chen@huawei.com> wrote: > > >> Only one of the options -s and -f can be used. When -f is used, > > >> the fd is created externally and does not need to be closed. > > So somebody is really using the 9p proxy driver for something; interesting. > > > > > > > The process running virtfs-proxy-helper has its own copy of > > > the fd inherited from its parent. And this fd will be closed > > > eventually when the process terminates. > > > > > >> When -s is used, a new socket fd is created, and this socket fd > > >> needs to be closed at the end of main(). > > > > > > Same here, the new socket fd is closed when the process > > > terminates. > > Does it? I haven't reviewed much of the 9p proxy code yet, however if chroot() > fails for instance, the fd would leak right now, wouldn't it? > This is done just at the end of main()... the leak won't last long. > Or was your argument that it's the OS's job to free any file descriptor > automatically on process terminations in general? > That's exactly my point. The only justification that'd deserve to be in the changelog of such a patch is something like "because this is good practice to rollback in case code moves to another function than main()". > > IMO, it's best to explicitly release resources before the process > > terminates, just as the variable 'rpath' is explicitly freed in main(), > > so socket fd also needs to be explicitly closed here. > > > > Looking forward to your reply. > > > > > The only justification to merge such a change would be if > > > the code was sitting in some other function, in which > > > case we should indeed do proper rollback. But it is main() > > > here, so this patch isn't needed. > > > > > >> Reported-by: Euler Robot <euler.robot@huawei.com> > > > > > > Can you provide a copy of the report in case I'm > > > missing something ? > > > > Our codecheck tool reports a resource leak here, which is relatively simple, > > like the one below, I did not attach it. > > > > --------------------- > > "Resource leak: sock" > > --------------------- > > Yeah, not very helpful that output. > Indeed :D > > > > Thanks, > > Alex > > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 18:27 ` Greg Kurz (?) @ 2020-11-26 18:44 ` Christian Schoenebeck 2020-11-27 9:10 ` Greg Kurz -1 siblings, 1 reply; 18+ messages in thread From: Christian Schoenebeck @ 2020-11-26 18:44 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz, Alex Chen, qemu-trivial, zhang.zhanghailiang On Donnerstag, 26. November 2020 19:27:19 CET Greg Kurz wrote: > On Thu, 26 Nov 2020 18:52:39 +0100 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote: > > > Hi Greg, > > > > > > Thanks for your review. > > > > > > On 2020/11/26 20:07, Greg Kurz wrote: > > > > On Thu, 26 Nov 2020 10:16:24 +0000 > > > > > > > > Alex Chen <alex.chen@huawei.com> wrote: > > > >> Only one of the options -s and -f can be used. When -f is used, > > > >> the fd is created externally and does not need to be closed. > > > > So somebody is really using the 9p proxy driver for something; > > interesting. > > > > > > The process running virtfs-proxy-helper has its own copy of > > > > the fd inherited from its parent. And this fd will be closed > > > > eventually when the process terminates. > > > > > > > >> When -s is used, a new socket fd is created, and this socket fd > > > >> needs to be closed at the end of main(). > > > > > > > > Same here, the new socket fd is closed when the process > > > > terminates. > > > > Does it? I haven't reviewed much of the 9p proxy code yet, however if > > chroot() fails for instance, the fd would leak right now, wouldn't it? > > This is done just at the end of main()... the leak won't last long. > > > Or was your argument that it's the OS's job to free any file descriptor > > automatically on process terminations in general? > > That's exactly my point. > > The only justification that'd deserve to be in the changelog of > such a patch is something like "because this is good practice > to rollback in case code moves to another function than main()". Well, the actual motivation was rather a pragmatic one: to shut up a sanitizer's false positive, which I can understand. Another option would be using a global variable for the fd instead of a temporary on stack. That should shut up the sanitizer as well and would not introduce change to the program flow. I leave that up to Greg to decide whether or not to handle this. I'm Switzerland on this one. Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() 2020-11-26 18:44 ` Christian Schoenebeck @ 2020-11-27 9:10 ` Greg Kurz 0 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-27 9:10 UTC (permalink / raw) To: Christian Schoenebeck Cc: qemu-devel, Alex Chen, qemu-trivial, zhang.zhanghailiang On Thu, 26 Nov 2020 19:44:24 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: [...] > > The only justification that'd deserve to be in the changelog of > > such a patch is something like "because this is good practice > > to rollback in case code moves to another function than main()". > > Well, the actual motivation was rather a pragmatic one: to shut up a > sanitizer's false positive, which I can understand. > Yes, this should also be mentioned in the changelog. > Another option would be using a global variable for the fd instead of a > temporary on stack. That should shut up the sanitizer as well and would not > introduce change to the program flow. > Using the same sock variable for an fd that is either passed to us or that we create is a very poor programming choice actually... :( So if the motivation is just to make "Euler Robot" happy and this can be addressed as you suggest, I personally prefer that rather than piling up fixes on broken code. > I leave that up to Greg to decide whether or not to handle this. I'm > Switzerland on this one. > This won't go into QEMU 5.2 anyway since we only merge fixes for critical bugs or regressions at this point. No hurry to decide anything now :) > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main() @ 2020-11-27 9:10 ` Greg Kurz 0 siblings, 0 replies; 18+ messages in thread From: Greg Kurz @ 2020-11-27 9:10 UTC (permalink / raw) To: Christian Schoenebeck Cc: Alex Chen, qemu-trivial, qemu-devel, zhang.zhanghailiang On Thu, 26 Nov 2020 19:44:24 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: [...] > > The only justification that'd deserve to be in the changelog of > > such a patch is something like "because this is good practice > > to rollback in case code moves to another function than main()". > > Well, the actual motivation was rather a pragmatic one: to shut up a > sanitizer's false positive, which I can understand. > Yes, this should also be mentioned in the changelog. > Another option would be using a global variable for the fd instead of a > temporary on stack. That should shut up the sanitizer as well and would not > introduce change to the program flow. > Using the same sock variable for an fd that is either passed to us or that we create is a very poor programming choice actually... :( So if the motivation is just to make "Euler Robot" happy and this can be addressed as you suggest, I personally prefer that rather than piling up fixes on broken code. > I leave that up to Greg to decide whether or not to handle this. I'm > Switzerland on this one. > This won't go into QEMU 5.2 anyway since we only merge fixes for critical bugs or regressions at this point. No hurry to decide anything now :) > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-11-27 9:12 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-26 10:16 [PATCH] virtfs-proxy-helper: Fix a resource leak in main() Alex Chen 2020-11-26 10:16 ` Alex Chen 2020-11-26 10:50 ` Li Qiang 2020-11-26 10:50 ` Li Qiang 2020-11-26 11:40 ` Alex Chen 2020-11-26 11:40 ` Alex Chen 2020-11-26 15:04 ` Li Qiang 2020-11-26 15:04 ` Li Qiang 2020-11-26 12:07 ` Greg Kurz 2020-11-26 12:07 ` Greg Kurz 2020-11-26 13:15 ` Alex Chen 2020-11-26 13:15 ` Alex Chen 2020-11-26 17:52 ` Christian Schoenebeck 2020-11-26 18:27 ` Greg Kurz 2020-11-26 18:27 ` Greg Kurz 2020-11-26 18:44 ` Christian Schoenebeck 2020-11-27 9:10 ` Greg Kurz 2020-11-27 9:10 ` Greg Kurz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.