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

[Martin C. <mc...@re...>]

On  Tue  2025-05-20  10:40 , Mark Wielaard wrote:
> Hi,
> 
> On Tue, May 20, 2025 at 07:38:58AM +0200, zz...@ge... wrote:
> > 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
> > [...]
> > >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.
> 
> Yeah, my fault. I tried to keep the logic the same by removing the
> safe_to_deref from the if clause as Martin originally
> suggested. Forgetting that also changes the logic flow :{ And then it
> was only tested against the ltp testsuite not the normal
> regtests. Apologies.

Lesson learned.  Now I can see how my update regressed
none/tests/rlimit{,64}_nofile.

Sorry about that.

Thanks for the fix!

Martin