Bug 424012

Summary:	Valgrind crashes with readv/writev having invalid but not NULL arg2 iovec pointer
Product:	[Developer tools] valgrind	Reporter:	Paul Floyd <pjfloyd>
Component:	general	Assignee:	Julian Seward <jseward>
Status:	RESOLVED FIXED
Severity:	crash	CC:	mark
Priority:	NOR
Version First Reported In:	3.15 SVN
Target Milestone:	---
Platform:	Compiled Sources
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:
Attachments:	Patch to solve the problem and update regtest + expected Update patch to have less inconsistent indentation

Description Paul Floyd 2020-07-08 21:38:33 UTC

SUMMARY

Valgrind crashes with readv/writev having invalid but not NULL arg2 iovec pointer.

This can be illustrated with a small modification to the memcheck/tests/writev1 testcase which gives:


==11135== Memcheck, a memory error detector
==11135== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11135== Using Valgrind-3.16.0.GIT and LibVEX; rerun with -h for copyright info
==11135== Command: memcheck/tests/writev1
==11135== 
Test file created.
==11135== Syscall param writev(vector[...]) points to unaddressable byte(s)
==11135==    at 0x4F34D60: __writev_nocancel (in /usr/lib64/libc-2.23.so)
==11135==    by 0x400A65: main (writev1.c:56)
==11135==  Address 0xffffffffffffffff is not stack'd, malloc'd or (recently) free'd
==11135== 
Received EFAULT as expected
Received EINVAL as expected
Received EINVAL as expected
==11135== Syscall param writev(vector) points to unaddressable byte(s)
==11135==    at 0x4F34D60: __writev_nocancel (in /usr/lib64/libc-2.23.so)
==11135==    by 0x400C59: main (writev1.c:85)
==11135==  Address 0x1 is not stack'd, malloc'd or (recently) free'd
==11135== 
--11135-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--11135-- si_code=1;  Faulting address: 0x1;  sp: 0x1002da9d40

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

host stacktrace:
==11135==    at 0x581063D1: vgSysWrap_generic_sys_writev_before (syswrap-generic.c:4651)
==11135==    by 0x580F6E96: vgPlain_client_syscall (syswrap-main.c:1914)
==11135==    by 0x580F3632: handle_syscall (scheduler.c:1208)
==11135==    by 0x580F4E16: vgPlain_scheduler (scheduler.c:1526)
==11135==    by 0x5814A026: thread_wrapper (syswrap-linux.c:101)
==11135==    by 0x5814A026: run_a_thread_NORETURN (syswrap-linux.c:154)

sched status:
  running_tid=1

Thread 1: status = VgTs_Runnable syscall 20 (lwpid 11135)
==11135==    at 0x4F34D60: __writev_nocancel (in /usr/lib64/libc-2.23.so)
==11135==    by 0x400C59: main (writev1.c:85)
client stack range: [0x1FFEFFE000 0x1FFF000FFF] client SP: 0x1FFF000268
valgrind stack range: [0x1002CAA000 0x1002DA9FFF] top usage: 7312 of 1048576


ADDITIONAL INFORMATION

There's a comment in the syswrap-main source code which is quite
accurate:
/* ToDo: don't do any of the following if the vector is invalid */

I will attach a patch shortly.

Comment 1 Paul Floyd 2020-07-08 21:44:26 UTC

Created attachment 129990 [details]
Patch to solve the problem and update regtest + expected

Comment 2 Paul Floyd 2020-07-08 21:45:45 UTC

With the attached patch

paulf> perl tests/vg_regtest memcheck/tests/writev1
writev1:         valgrind   -q ./writev1 

== 1 test, 0 stderr failures, 0 stdout failures, 0 stderrB failures, 0 stdoutB failures, 0 post failures ==

Comment 3 Mark Wielaard 2020-11-07 22:21:28 UTC

The patch and the testcase look good to me. Thanks for doing this.

Note that the indentation of the first nested if-else in the testcase seems a little off.

Comment 4 Paul Floyd 2020-11-08 17:18:10 UTC

Created attachment 133152 [details]
Update patch to have less inconsistent indentation

I've tried to make the indentation more consistent. The file has a fairly random mix of tabs and spaces, and the tab alignment is 8 spaces.

Comment 5 Mark Wielaard 2020-11-08 19:23:58 UTC

(In reply to Paul Floyd from comment #4)
> Created attachment 133152 [details]
> Update patch to have less inconsistent indentation
> 
> I've tried to make the indentation more consistent. The file has a fairly
> random mix of tabs and spaces, and the tab alignment is 8 spaces.

It does look consistent now to me. Thanks.

Some tests (like this one) don't really follow the indentation of the main code. Don't worry too much about it. It is just convenient if the indentation in one file is consistent. Sorry if I made you do extra work.

Comment 6 Mark Wielaard 2021-02-20 15:34:45 UTC

commit 3aa3482774cf99ba23b7a6eac97d17cf143af5aa
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Mon Nov 9 09:30:31 2020 +0100

    Bug 424012 - fix crash if readv/writev have invalid but not NULL arg2 iovec pointer