Bug 408414 - Add support for missing for preadv2 and pwritev2 syscalls
Summary: Add support for missing for preadv2 and pwritev2 syscalls
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-07 10:49 UTC by Alexandra Hajkova
Modified: 2019-07-03 08:30 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
new version of the patch (19.05 KB, patch)
2019-06-11 12:00 UTC, Alexandra Hajkova
Details
patch (25.05 KB, patch)
2019-06-12 17:56 UTC, Alexandra Hajkova
Details
new version of the patch (25.85 KB, patch)
2019-06-19 11:16 UTC, Alexandra Hajkova
Details
Add support for preadv2 and pwritev2 syscalls (24.83 KB, text/plain)
2019-06-21 15:51 UTC, Mark Wielaard
Details
patch (28.43 KB, patch)
2019-07-02 15:56 UTC, Alexandra Hajkova
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandra Hajkova 2019-06-07 10:49:42 UTC
SUMMARY


STEPS TO REPRODUCE
1. compile glibc
2. run from the build dir:
 ./testrun.sh --tool=valgrind misc/tst-preadvwritev2 --direct


OBSERVED RESULT
 Memcheck, a memory error detector

...
--8186-- WARNING: unhandled amd64-linux syscall: 327
--8186-- You may be able to write your own handler.
--8186-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--8186-- Nevertheless we consider this a bug.  Please report
--8186-- it at http://valgrind.org/support/bug_reports.html.

...

==8186== Syscall param pwritev(vector[...]) points to uninitialised byte(s)
==8186==    at 0x4D01369: pwritev (pwritev64.c:30)
==8186==    by 0x4019CE: do_test_with_invalid_iov (tst-preadvwritev2-common.c:80)
==8186==    by 0x401F15: do_test (tst-preadvwritev2.c:34)
==8186==    by 0x403061: support_test_main (support_test_main.c:350)
==8186==    by 0x401618: main (test-driver.c:168)
==8186==  Address 0x1ffeffb040 is on thread 1's stack
==8186==  in frame #1, created by do_test_with_invalid_iov (tst-preadvwritev2-common.c:70)
==8186== 
--8186-- WARNING: unhandled amd64-linux syscall: 327
--8186-- You may be able to write your own handler.
--8186-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--8186-- Nevertheless we consider this a bug.  Please report
--8186-- it at http://valgrind.org/support/bug_reports.html.
--8186-- WARNING: unhandled amd64-linux syscall: 328
...

RESULTS with proposed patch:
....

==30023== Syscall param pwritev2(vector[...]) points to uninitialised byte(s)
==30023==    at 0x4D0159D: pwritev2 (pwritev64v2.c:31)
==30023==    by 0x401CEC: do_test_with_invalid_flags (tst-preadvwritev2-common.c:119)
==30023==    by 0x401CEC: do_test (tst-preadvwritev2.c:31)
==30023==    by 0x403061: support_test_main (support_test_main.c:350)
==30023==    by 0x401618: main (test-driver.c:168)
==30023==  Address 0x1ffefff0f0 is on thread 1's stack
==30023==  in frame #1, created by do_test (tst-preadvwritev2-common.c:30)
==30023== 
==30023== Warning: invalid file descriptor -1 in syscall preadv2()
==30023== Warning: invalid file descriptor -1 in syscall pwritev2()
==30023== Warning: invalid file descriptor -1 in syscall preadv2()
==30023== Warning: invalid file descriptor -1 in syscall pwritev2()
==30023== Syscall param preadv2(vector[...]) points to unaddressable byte(s)
==30023==    at 0x4D0143D: preadv2 (preadv64v2.c:31)
==30023==    by 0x401988: do_test_with_invalid_iov (tst-preadvwritev2-common.c:78)
==30023==    by 0x401F15: do_test (tst-preadvwritev2.c:34)
==30023==    by 0x403061: support_test_main (support_test_main.c:350)
==30023==    by 0x401618: main (test-driver.c:168)
==30023==  Address 0x1fff001000 is not stack'd, malloc'd or (recently) free'd
==30023== 
==30023== Syscall param pwritev2(vector[...]) points to uninitialised byte(s)
==30023==    at 0x4D0159D: pwritev2 (pwritev64v2.c:31)
==30023==    by 0x4019CE: do_test_with_invalid_iov (tst-preadvwritev2-common.c:80)
==30023==    by 0x401F15: do_test (tst-preadvwritev2.c:34)
==30023==    by 0x403061: support_test_main (support_test_main.c:350)
==30023==    by 0x401618: main (test-driver.c:168)
==30023==  Address 0x1ffeffb020 is on thread 1's stack
==30023==  in frame #1, created by do_test_with_invalid_iov (tst-preadvwritev2-common.c:70)
==30023== 
==30023== Syscall param preadv2(vector[...]) points to unaddressable byte(s)
==30023==    at 0x4D0143D: preadv2 (preadv64v2.c:31)
==30023==    by 0x401A0F: do_test_with_invalid_iov (tst-preadvwritev2-common.c:85)
==30023==    by 0x401F15: do_test (tst-preadvwritev2.c:34)
==30023==    by 0x403061: support_test_main (support_test_main.c:350)
==30023==    by 0x401618: main (test-driver.c:168)
==30023==  Address 0x1fff001000 is not stack'd, malloc'd or (recently) free'd
==30023== 
==30023== Syscall param pwritev2(vector[...]) points to uninitialised byte(s)
==30023==    at 0x4D0159D: pwritev2 (pwritev64v2.c:31)
==30023==    by 0x401A54: do_test_with_invalid_iov (tst-preadvwritev2-common.c:87)
==30023==    by 0x401F15: do_test (tst-preadvwritev2.c:34)
==30023==    by 0x403061: support_test_main (support_test_main.c:350)
==30023==    by 0x401618: main (test-driver.c:168)
==30023==  Address 0x1ffeffb020 is on thread 1's stack
==30023==  in frame #1, created by do_test_with_invalid_iov (tst-preadvwritev2-common.c:70)
==30023== 
==30023== 

....

Proposed patch on github: https://github.com/sasshka/valgrind/commit/a671972cbce09a4e6d03905d4fd70c1ba84807af
Comment 1 Florian Weimer 2019-06-07 10:56:09 UTC
To be clear, the glibc test uses invalid arguments, so these errors are expected.
Comment 2 Alexandra Hajkova 2019-06-11 12:00:45 UTC
Created attachment 120783 [details]
new version of the patch

The new version of the patch, reworked after irc discussion with Mark Wielaard, with the sys-preadv2_pwritev2.stderr.exp added
Comment 3 Alexandra Hajkova 2019-06-12 17:56:33 UTC
Created attachment 120820 [details]
patch
Comment 4 Alexandra Hajkova 2019-06-19 11:16:01 UTC
Created attachment 120996 [details]
new version of the patch
Comment 5 Mark Wielaard 2019-06-21 14:12:24 UTC
We discussed the last version of this patch on irc.  It had mismatched .exp files, but this was easily fixed.  I did update the printing of the vector from vector[...] to include the actual index.

The new test case is nice because it shows the current valgrind pwritev handling can crash:

$ valgrind -q memcheck/tests/linux/sys-preadv_pwritev
==21111== Syscall param pwritev(vector) points to unaddressable byte(s)
==21111==    at 0x4F2AC7A: pwritev (pwritev.c:42)
==21111==    by 0x400BDE: main (sys-preadv_pwritev.c:53)
==21111==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==21111== 
==21111== Syscall param pwritev(vector) points to unaddressable byte(s)
==21111==    at 0x4F2AC7A: pwritev (pwritev.c:42)
==21111==    by 0x400BF6: main (sys-preadv_pwritev.c:54)
==21111==  Address 0xffffffffffffffff is not stack'd, malloc'd or (recently) free'd
==21111== 
--21111-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--21111-- si_code=1;  Faulting address: 0xFFFFFFFFFFFFFFFF;  sp: 0x1003283d70

valgrind: the 'impossible' happened:
   Killed by fatal signal

host stacktrace:
==21111==    at 0x580B9FFC: vgSysWrap_linux_sys_pwritev_before (syswrap-linux.c:5422)
==21111==    by 0x5809767F: vgPlain_client_syscall (syswrap-main.c:1857)
==21111==    by 0x58094372: handle_syscall (scheduler.c:1126)
==21111==    by 0x58095786: vgPlain_scheduler (scheduler.c:1443)
==21111==    by 0x580A4C2A: UnknownInlinedFun (syswrap-linux.c:103)
==21111==    by 0x580A4C2A: run_a_thread_NORETURN (syswrap-linux.c:156)

sched status:
  running_tid=1

Thread 1: status = VgTs_Runnable (lwpid 21111)
==21111==    at 0x4F2AC7A: pwritev (pwritev.c:42)
==21111==    by 0x400BF6: main (sys-preadv_pwritev.c:54)

This is fixed with Sasha's patch.

The testcase also tries to test handling an uninitialized flag argument for the v2 syscall.
But to our surprise this didn't work... The flag always came out as defined zero.
Turns out glibc does this deliberately on 64bit architectures because the kernel does actually have a low_offset and high_offset argument, but ignores the high_offset/assumes it is zero...
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=601cc11d054ae4b5e9b5babec3d8e4667a2cb9b5

There is a fixed up version here:
https://code.wildebeest.org/git/user/mjw/valgrind/commit/?h=pvrw2

Not tested yet were non-x86_64 architectures, but I think we should just hook up the other linux arches (the nightly builders will alert us if they behave differently).
Comment 6 Mark Wielaard 2019-06-21 15:51:50 UTC
Created attachment 121061 [details]
Add support for preadv2 and pwritev2 syscalls

This is the current patch from https://code.wildebeest.org/git/user/mjw/valgrind/commit/?h=pvrw2
Comment 7 Alexandra Hajkova 2019-07-02 15:56:42 UTC
Created attachment 121289 [details]
patch
Comment 8 Mark Wielaard 2019-07-02 22:34:22 UTC
(In reply to Alexandra Hajkova from comment #7)
> Created attachment 121289 [details]
> patch

This looks great. Thanks for the polish and wiring up the other arches.
I haven't been able to test them all. But the nightly builders will catch any issues.
Pushed to valgrind git master as:

commit b0861063a8d2a55bb7423e90d26806bab0f78a12 (HEAD -> master, origin/master, origin/HEAD)
Author: Alexandra Hájková <ahajkova@redhat.com>
Date:   Tue Jun 4 13:47:14 2019 +0200

    Add support for preadv2 and pwritev2 syscalls
    
    Support for amd64, x86 - 64 and 32 bit, arm64, ppc64, ppc64le,
    s390x, mips64. This should work identically on all
    arches, tested on x86 32bit and 64bit one, but enabled on all.
    
    Refactor the code to be reusable between old/new syscalls. Resolve TODO
    items in the code. Add the testcase for the preadv2/pwritev2 and also
    add the (similar) testcase for the older preadv/pwritev syscalls.
    
    Trying to test handling an uninitialized flag argument for the v2 syscalls
    does not work because the flag always comes out as defined zero.
    Turns out glibc does this deliberately on 64bit architectures because
    the kernel does actually have a low_offset and high_offset argument, but
    ignores the high_offset/assumes it is zero.
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=601cc11d054ae4b5e9b5babec3d8e4667a2cb9b5
    
    https://bugs.kde.org/408414
Comment 9 Mark Wielaard 2019-07-03 08:30:00 UTC
Small followup for arm64 typo.

commit 514f899388e05142513ff3f679a9e0131145e34e
Author: Mark Wielaard <mark@klomp.org>
Date:   Wed Jul 3 10:27:17 2019 +0200

    Hook up preadv2 and pwritev2 correctly for arm64.
    
    Use the correct generic linux sys wrapper.
    
    Followup for https://bugs.kde.org/408414