Re: [Valgrind-developers] [valgrind] PR504341: Prevent LTP setrlimit05 syscall test from crashing valgrind

[<zz...@ge...>]

This change looks strange to me.

Am 19.05.25 um 18:53 schrieb Mark Wielaard:
> https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=859d267a456c2921772f0c957bf24f463c51bd93
> 
> commit 859d267a456c2921772f0c957bf24f463c51bd93
> Author: Martin Cermak <mc...@re...>
> Date:   Mon May 19 11:45:04 2025 +0200
> 
>      PR504341: Prevent LTP setrlimit05 syscall test from crashing valgrind
>      
>      Prevent ltp/testcases/kernel/syscalls/setrlimit/setrlimit05 testcase
>      from crashing valgrind when passing 0xffffffffffff as ARG3 and then
>      trying to dereference it.
>      
>      https://bugs.kde.org/show_bug.cgi?id=504341
> 
> Diff:
> ---
>   NEWS                                |  1 +
>   coregrind/m_syswrap/syswrap-linux.c | 14 ++++++++------
>   2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index d6fbbb41b9..7bb9a79d10 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -33,6 +33,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
>   501741  syscall cachestat not wrapped
>   503969  Make test results of make ltpchecks compatible with bunsen
>   504265  FreeBSD: missing syscall wrappers for fchroot and setcred
> +504341  Valgrind killed by LTP syscall testcase setrlimit05
>   
>   To see details of a given bug, visit
>     https://bugs.kde.org/show_bug.cgi?id=XXXXXX
> diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c
> index d4653d0273..470635f565 100644
> --- a/coregrind/m_syswrap/syswrap-linux.c
> +++ b/coregrind/m_syswrap/syswrap-linux.c
> @@ -2300,12 +2300,14 @@ PRE(sys_prlimit64)
>      if (ARG4)
>         PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct vki_rlimit64) );
>   
> -   if (ARG3 &&
> -       ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
> -        > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
> -      SET_STATUS_Failure( VKI_EINVAL );
> -   }
> -   else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
> +   if (ARG3) {

Changing the condition from (ARG3 && rlim_cur > rlim_max) to just (ARG3) 
changes the logic. So some branches below are not executed when they 
should. The switch inside "else if (ARG1 == 0 || ARG1 == VG_(getpid)())" 
is the main part of prlimit64.

The testsuite catches this:

$ perl tests/vg_regtest none/tests/rlimit{,64}_nofile
rlimit_nofile:   valgrind   ./rlimit_nofile
*** rlimit_nofile failed (stderr) ***
rlimit64_nofile: valgrind   ./rlimit64_nofile
*** rlimit64_nofile failed (stderr) ***

== 2 tests, 2 stderr failures, 0 stdout failures, 0 stderrB failures, 0 
stdoutB failures, 0 post failures ==
none/tests/rlimit_nofile                 (stderr)
none/tests/rlimit64_nofile               (stderr)

$ cat none/tests/rlimit64_nofile.stderr.diff
--- rlimit64_nofile.stderr.exp  2023-10-27 09:53:13.809659010 +0200
+++ rlimit64_nofile.stderr.out  2025-05-20 07:35:14.513269310 +0200
@@ -1,2 +1,3 @@

+setrlimit64 changing hardlimit must return -1

The simplest fix I can image is to check the pointers before entering 
the real logic if-else-if block:

--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -2295,19 +2295,28 @@ PRE(sys_prlimit64)
                   vki_pid_t, pid, unsigned int, resource,
                   const struct rlimit64 *, new_rlim,
                   struct rlimit64 *, old_rlim);
-   if (ARG3)
+   if (ARG3) {
        PRE_MEM_READ( "rlimit64(new_rlim)", ARG3, sizeof(struct 
vki_rlimit64) );
-   if (ARG4)
-      PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct 
vki_rlimit64) );
+      if (! ML_(safe_to_deref)((void *)(Addr)ARG3, sizeof(struct 
vki_rlimit64))) {
+         SET_STATUS_Failure ( VKI_EFAULT );
+        return;
+      }
+   }

-   if (ARG3) {
-      if (ML_(safe_to_deref)( (void*)(Addr)ARG3, sizeof(struct 
vki_rlimit64) )) {
-         if (((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
-              > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
-            SET_STATUS_Failure( VKI_EINVAL );
-         }
+   if (ARG4) {
+      PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct 
vki_rlimit64) );
+      if (! ML_(safe_to_deref)((void *)(Addr)ARG4, sizeof(struct 
vki_rlimit64))) {
+         SET_STATUS_Failure ( VKI_EFAULT );
+        return;
        }
-   } else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
+   }
+
+   if (ARG3 &&
+       ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
+        > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
+      SET_STATUS_Failure( VKI_EINVAL );
+   }
+   else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
        switch (ARG2) {
        case VKI_RLIMIT_NOFILE:
           SET_STATUS_Success( 0 );



Regards
Matthias