| Message ID | 1604925271-4811-1-git-send-email-zhufy.jy@cn.fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pan/ltp-pan.c: fix file descriptors leaks | expand |
Hi! > ltp-pan will leak file descriptors of fopen() into the child process > of the test case, fix this problem by using mode "e" for fopen(). Looks good, however this is supported since glibc 2.7 and it does not seem to be supported on musl libc either. I guess that it would be better just to close these files after a fork in the runchild() function, but that would mean that we would have to pass all these files as paramters to the function as well.
Hi Cyril, Feiyu > Hi! >> ltp-pan will leak file descriptors of fopen() into the child process >> of the test case, fix this problem by using mode "e" for fopen(). > > Looks good, however this is supported since glibc 2.7 and it does not > seem to be supported on musl libc either. > Yes, musl-libc doesn't support "e" mode for fopen[1]. [1]https://git.musl-libc.org/cgit/musl/tree/src/stdio/fopen.c > I guess that it would be better just to close these files after a fork > in the runchild() function, but that would mean that we would have to > pass all these files as paramters to the function as well. +1 >
Hi Cyril > Hi Cyril, Feiyu >> Hi! >>> ltp-pan will leak file descriptors of fopen() into the child process >>> of the test case, fix this problem by using mode "e" for fopen(). >> >> Looks good, however this is supported since glibc 2.7 and it does not >> seem to be supported on musl libc either. >> > Yes, musl-libc doesn't support "e" mode for fopen[1]. Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to fopen and fdopen"). https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 It is about 8 years since musl libc fopen() supports "e". glibc2.7 fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? > > [1]https://git.musl-libc.org/cgit/musl/tree/src/stdio/fopen.c >> I guess that it would be better just to close these files after a fork >> in the runchild() function, but that would mean that we would have to >> pass all these files as paramters to the function as well. > +1 >> > > > >
Hi! > > Yes, musl-libc doesn't support "e" mode for fopen[1]. > Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since > 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to > fopen and fdopen"). > > https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 > > It is about 8 years since musl libc fopen() supports "e". glibc2.7 > fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? To be honest I haven't had used ltp-pan for last two years, so if that change works for everyone still using it, then we can go ahead with it.
Hi Cyril > Hi! >>> Yes, musl-libc doesn't support "e" mode for fopen[1]. >> Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since >> 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to >> fopen and fdopen"). >> >> https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 >> >> It is about 8 years since musl libc fopen() supports "e". glibc2.7 >> fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? > > To be honest I haven't had used ltp-pan for last two years, so if that > change works for everyone still using it, then we can go ahead with it. OK. I will wait a week. If nobody has objection, I will merge it. Best Regards Yang Xu >
Hi Petr > Hi Cyril >> Hi! >>>> Yes, musl-libc doesn't support "e" mode for fopen[1]. >>> Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since >>> 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to >>> fopen and fdopen"). >>> >>> https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 >>> >>> >>> It is about 8 years since musl libc fopen() supports "e". glibc2.7 >>> fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? >> >> To be honest I haven't had used ltp-pan for last two years, so if that >> change works for everyone still using it, then we can go ahead with it. > OK. I will wait a week. If nobody has objection, I will merge it. I plan to merge this patch today. Before it, I want to listen some advise from you( IMO, you know musl-libc a lot and other libc on embedded system). > > Best Regards > Yang Xu >> >
Hi Xu, > Hi Petr > > Hi Cyril > > > Hi! > > > > > Yes, musl-libc doesn't support "e" mode for fopen[1]. > > > > Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since > > > > 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to > > > > fopen and fdopen"). > > > > https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 > > > > It is about 8 years since musl libc fopen() supports "e". glibc2.7 > > > > fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? > > > To be honest I haven't had used ltp-pan for last two years, so if that > > > change works for everyone still using it, then we can go ahead with it. > > OK. I will wait a week. If nobody has objection, I will merge it. > I plan to merge this patch today. Before it, I want to listen some advise > from you( IMO, you know musl-libc a lot and other libc on embedded system). Acked-by: Petr Vorel <pvorel@suse.cz> Should be safe. Kind regards, Petr > > Best Regards > > Yang Xu
Hi Feiyu Pushed, thanks! Best Regards Yang Xu > Hi Xu, > >> Hi Petr >>> Hi Cyril >>>> Hi! >>>>>> Yes, musl-libc doesn't support "e" mode for fopen[1]. >>>>> Sorry, I ignore __fmodeflags function, musl libc supports "e" mode since >>>>> 0.9.7 after this commit 8582a6e9f ("add 'e' modifier (close-on-exec) to >>>>> fopen and fdopen"). > >>>>> https://git.musl-libc.org/cgit/musl/commit/src?id=8582a6e9f25dd7b87d72961f58008052a4cac473 > > >>>>> It is about 8 years since musl libc fopen() supports "e". glibc2.7 >>>>> fopen() supports "e" is about 13 years. Maybe we can use "e" mode now? > >>>> To be honest I haven't had used ltp-pan for last two years, so if that >>>> change works for everyone still using it, then we can go ahead with it. >>> OK. I will wait a week. If nobody has objection, I will merge it. >> I plan to merge this patch today. Before it, I want to listen some advise >> from you( IMO, you know musl-libc a lot and other libc on embedded system). > > Acked-by: Petr Vorel<pvorel@suse.cz> > > Should be safe. > > Kind regards, > Petr > >>> Best Regards >>> Yang Xu > > > . >
diff --git a/pan/ltp-pan.c b/pan/ltp-pan.c index 8b9fbe5..25e554f 100644 --- a/pan/ltp-pan.c +++ b/pan/ltp-pan.c @@ -336,7 +336,7 @@ int main(int argc, char **argv) if (!strcmp(logfilename, "-")) { logfile = stdout; } else { - if ((logfile = fopen(logfilename, "a+")) == NULL) { + if ((logfile = fopen(logfilename, "a+e")) == NULL) { fprintf(stderr, "pan(%s): Error %s (%d) opening log file '%s'\n", panname, strerror(errno), errno, @@ -453,7 +453,7 @@ int main(int argc, char **argv) } if (failcmdfilename) { - if (!(failcmdfile = fopen(failcmdfilename, "a+"))) { + if (!(failcmdfile = fopen(failcmdfilename, "a+e"))) { fprintf(stderr, "pan(%s): Error %s (%d) opening fail cmd file '%s'\n", panname, strerror(errno), errno, @@ -463,7 +463,7 @@ int main(int argc, char **argv) } if (tconfcmdfilename) { - tconfcmdfile = fopen(tconfcmdfilename, "a+"); + tconfcmdfile = fopen(tconfcmdfilename, "a+e"); if (!tconfcmdfile) { fprintf(stderr, "pan(%s): Error %s (%d) opening " "tconf cmd file '%s'\n", panname,
ltp-pan will leak file descriptors of fopen() into the child process of the test case, fix this problem by using mode "e" for fopen(). Signed-off-by: Feiyu Zhu <zhufy.jy@cn.fujitsu.com> --- pan/ltp-pan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)