diff mbox series

[v2] linux: wait4: Fix incorrect return value comparison

Message ID 20200407054010.2573333-1-alistair.francis@wdc.com
State New, archived
Headers
Series [v2] linux: wait4: Fix incorrect return value comparison |

Commit Message

Alistair Francis April 7, 2020, 5:40 a.m. UTC 
  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(-)

Comments

Andreas Schwab April 7, 2020, 7:35 a.m. UTC | #1 
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.
Florian Weimer April 7, 2020, 11:23 a.m. UTC | #2 
* 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
Adhemerval Zanella April 7, 2020, 1:24 p.m. UTC | #3 
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.
Alistair Francis April 7, 2020, 2:47 p.m. UTC | #4 
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
Florian Weimer April 9, 2020, 3:35 p.m. UTC | #5 
* 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
Adhemerval Zanella April 9, 2020, 3:41 p.m. UTC | #6 
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.
Alistair Francis April 9, 2020, 8:13 p.m. UTC | #7 
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 mbox series

Patch

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;
 }