Message ID | 20200407054010.2573333-1-alistair.francis@wdc.com |
---|---|
State | New |
Headers | show |
Series | [v2] linux: wait4: Fix incorrect return value comparison | expand |
On Apr 06 2020, Alistair Francis via Libc-alpha wrote: > diff --git a/sysdeps/unix/sysv/linux/wait4.c b/sysdeps/unix/sysv/linux/wait4.c > index d14bd4da27..973af48951 100644 > --- a/sysdeps/unix/sysv/linux/wait4.c > +++ b/sysdeps/unix/sysv/linux/wait4.c > @@ -29,14 +29,16 @@ __wait4_time64 (pid_t pid, int *stat_loc, int options, struct __rusage64 *usage) > # if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 > return SYSCALL_CANCEL (wait4, pid, stat_loc, options, usage); > # else > - struct __rusage32 usage32; > - pid_t ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32); > + pid_t ret; > > - if (ret != 0) > - return ret; > + if (usage != NULL) { Please use GNU style indentation. Andreas.
* Alistair Francis via Libc-alpha: > Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced Typo: "Path" > two bugs: > - The usage32 struct was set if the wait4 syscall had an error. > - For 32-bit systems the usage struct was set even if it was specified > as NULL. > > This patch fixes the two issues. Would it make sense to include a test case? (Somehow, my earlier message did not make it it to the list.) Thanks, Florian
On 07/04/2020 08:23, Florian Weimer via Libc-alpha wrote: > * Alistair Francis via Libc-alpha: > >> Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced > > Typo: "Path" > >> two bugs: >> - The usage32 struct was set if the wait4 syscall had an error. >> - For 32-bit systems the usage struct was set even if it was specified >> as NULL. >> >> This patch fixes the two issues. > > Would it make sense to include a test case? > > (Somehow, my earlier message did not make it it to the list.) I think it would, specially now that glibc does more internally.
On Tue, Apr 7, 2020 at 6:25 AM Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > > On 07/04/2020 08:23, Florian Weimer via Libc-alpha wrote: > > * Alistair Francis via Libc-alpha: > > > >> Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced > > > > Typo: "Path" > > > >> two bugs: > >> - The usage32 struct was set if the wait4 syscall had an error. > >> - For 32-bit systems the usage struct was set even if it was specified > >> as NULL. > >> > >> This patch fixes the two issues. > > > > Would it make sense to include a test case? > > > > (Somehow, my earlier message did not make it it to the list.) > > I think it would, specially now that glibc does more internally. I agree. Would you like me to write it or does someone who knows more about the glibc tests like to do it? Alistair
* Alistair Francis via Libc-alpha: > On Tue, Apr 7, 2020 at 6:25 AM Adhemerval Zanella via Libc-alpha > <libc-alpha@sourceware.org> wrote: >> >> >> >> On 07/04/2020 08:23, Florian Weimer via Libc-alpha wrote: >> > * Alistair Francis via Libc-alpha: >> > >> >> Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced >> > >> > Typo: "Path" >> > >> >> two bugs: >> >> - The usage32 struct was set if the wait4 syscall had an error. >> >> - For 32-bit systems the usage struct was set even if it was specified >> >> as NULL. >> >> >> >> This patch fixes the two issues. >> > >> > Would it make sense to include a test case? >> > >> > (Somehow, my earlier message did not make it it to the list.) >> >> I think it would, specially now that glibc does more internally. > > I agree. > > Would you like me to write it or does someone who knows more about the > glibc tests like to do it? It looks like we should commit the patch sooner than later. We can work on the test case next week. Either I can teach you to how to write it, or I can write it myself. Would you please post a v3 with just the indentation fixes? Thanks, Florian
On 09/04/2020 12:35, Florian Weimer wrote: > * Alistair Francis via Libc-alpha: > >> On Tue, Apr 7, 2020 at 6:25 AM Adhemerval Zanella via Libc-alpha >> <libc-alpha@sourceware.org> wrote: >>> >>> >>> >>> On 07/04/2020 08:23, Florian Weimer via Libc-alpha wrote: >>>> * Alistair Francis via Libc-alpha: >>>> >>>>> Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced >>>> >>>> Typo: "Path" >>>> >>>>> two bugs: >>>>> - The usage32 struct was set if the wait4 syscall had an error. >>>>> - For 32-bit systems the usage struct was set even if it was specified >>>>> as NULL. >>>>> >>>>> This patch fixes the two issues. >>>> >>>> Would it make sense to include a test case? >>>> >>>> (Somehow, my earlier message did not make it it to the list.) >>> >>> I think it would, specially now that glibc does more internally. >> >> I agree. >> >> Would you like me to write it or does someone who knows more about the >> glibc tests like to do it? > > It looks like we should commit the patch sooner than later. > > We can work on the test case next week. Either I can teach you to how > to write it, or I can write it myself. > > Would you please post a v3 with just the indentation fixes? I think we can posix/tst-waitid.c as a starting point.
On Thu, Apr 9, 2020 at 8:36 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Alistair Francis via Libc-alpha: > > > On Tue, Apr 7, 2020 at 6:25 AM Adhemerval Zanella via Libc-alpha > > <libc-alpha@sourceware.org> wrote: > >> > >> > >> > >> On 07/04/2020 08:23, Florian Weimer via Libc-alpha wrote: > >> > * Alistair Francis via Libc-alpha: > >> > > >> >> Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced > >> > > >> > Typo: "Path" > >> > > >> >> two bugs: > >> >> - The usage32 struct was set if the wait4 syscall had an error. > >> >> - For 32-bit systems the usage struct was set even if it was specified > >> >> as NULL. > >> >> > >> >> This patch fixes the two issues. > >> > > >> > Would it make sense to include a test case? > >> > > >> > (Somehow, my earlier message did not make it it to the list.) > >> > >> I think it would, specially now that glibc does more internally. > > > > I agree. > > > > Would you like me to write it or does someone who knows more about the > > glibc tests like to do it? > > It looks like we should commit the patch sooner than later. Just posted v3. > > We can work on the test case next week. Either I can teach you to how > to write it, or I can write it myself. I also wrote a wait4 test. It's very simple but should have caught this issue. Let me know what you think. I couldn't easily think of anything else to add to the test case. Alistair > > Would you please post a v3 with just the indentation fixes? > > Thanks, > Florian >
diff --git a/sysdeps/unix/sysv/linux/wait4.c b/sysdeps/unix/sysv/linux/wait4.c index d14bd4da27..973af48951 100644 --- a/sysdeps/unix/sysv/linux/wait4.c +++ b/sysdeps/unix/sysv/linux/wait4.c @@ -29,14 +29,16 @@ __wait4_time64 (pid_t pid, int *stat_loc, int options, struct __rusage64 *usage) # if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 return SYSCALL_CANCEL (wait4, pid, stat_loc, options, usage); # else - struct __rusage32 usage32; - pid_t ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32); + pid_t ret; - if (ret != 0) - return ret; + if (usage != NULL) { + struct __rusage32 usage32; + ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32); - if (usage != NULL) - rusage32_to_rusage64 (&usage32, usage); + if (ret > 0) + rusage32_to_rusage64 (&usage32, usage); + } else + ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, NULL); return ret; # endif @@ -114,15 +116,17 @@ libc_hidden_def (__wait4_time64) pid_t __wait4 (pid_t pid, int *stat_loc, int options, struct rusage *usage) { - pid_t ret ; - struct __rusage64 usage64; + pid_t ret; - ret = __wait4_time64 (pid, stat_loc, options, &usage64); + if (usage != NULL) { + struct __rusage64 usage64; - if (ret != 0) - return ret; + ret = __wait4_time64 (pid, stat_loc, options, &usage64); - rusage64_to_rusage (&usage64, usage); + if (ret > 0) + rusage64_to_rusage (&usage64, usage); + } else + ret = __wait4_time64 (pid, stat_loc, options, NULL); return ret; }
Path 600f00b "linux: Use long time_t for wait4/getrusage" introduced two bugs: - The usage32 struct was set if the wait4 syscall had an error. - For 32-bit systems the usage struct was set even if it was specified as NULL. This patch fixes the two issues. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- sysdeps/unix/sysv/linux/wait4.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)