Bug 379039

Summary: syscall wrapper for prctl(PR_SET_NAME) must not check more than 16 bytes
Product: [Developer tools] valgrind Reporter: Peter (Stig) Edwards <thatsafunnyname.ra7qa>
Component: memcheckAssignee: Ivo Raisr <ivosh>
Status: ASSIGNED ---    
Severity: minor CC: ivosh
Priority: NOR    
Version: 3.12.0   
Target Milestone: ---   
Platform: RedHat Enterprise Linux   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: A hack of a patch that just duplicates the asciiz func with a _limit_len version.
patch utilizing VG_(strnlen) function
patch utilizing VG_(strnlen) function II.
patch utilizing VG_(strnlen) and VG_(strlcpy) functions III.

Description Peter (Stig) Edwards 2017-04-21 12:55:12 UTC
Hello and thank you for valgrind,
 
  This is a minor edge case.

  I noticed that with a non null terminated array of 16 chars passed to prctl with PR_SET_NAME, valgrind 3.12.0 ( and 3.11.0 , 3.10.0 , 3.8.1 and 3.5.0 ) memcheck ends up calling mc_is_defined_asciiz which checks a zero-terminated ascii string and reports the memory error "Syscall param prctl(set-name) points to uninitialised byte(s)".  I don't think the name *has* to be null terminated if it is 16 bytes (or more).

From the linux man page:

  "The name can be up to 16 bytes long, 
   including the terminating null byte.
   (If the length of the string, including the terminating null byte,
   exceeds 16 bytes, the string is silently truncated.)"

Reproducer:

# cat valgrind_memcheck_prctl.c
#include <sys/prctl.h>
int main(int argc, char *argv[]){
  char buf[16] = "1234567890123456";
  int ret = prctl(PR_SET_NAME, buf, 0, 0, 0);
  return ret;
}

# gcc -g valgrind_memcheck_prctl.c

# valgrind --track-origins=yes ./a.out
==143257== Memcheck, a memory error detector
==143257== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==143257== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==143257== Command: ./a.out
==143257==
==143257== Syscall param prctl(set-name) points to uninitialised byte(s)
==143257==    at 0x4F2AF8A: prctl (in /usr/lib64/libc-2.17.so)
==143257==    by 0x400580: main (valgrind_memcheck_prctl.c:4)
==143257==  Address 0xfff000260 is on thread 1's stack
==143257==  in frame #1, created by main (valgrind_memcheck_prctl.c:2)
==143257==  Uninitialised value was created by a stack allocation
==143257==    at 0x400530: main (valgrind_memcheck_prctl.c:2)
==143257==
==143257==
==143257== HEAP SUMMARY:
==143257==     in use at exit: 0 bytes in 0 blocks
==143257==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==143257==
==143257== All heap blocks were freed -- no leaks are possible
==143257==
==143257== For counts of detected and suppressed errors, rerun with: -v
==143257== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
 
Above is on a RHEL7 x86_64 host, using the valgrind-3.10.0-16.el7 RPM and gcc-4.8.5-4.el7.

Cheers,
Peter (Stig) Edwards
Comment 1 Ivo Raisr 2017-04-21 12:59:31 UTC
Yes, indeed. Kernel takes care to copy TASK_COMM_LEN at max.
Please could you supply a patch with the fix?
Comment 2 Peter (Stig) Edwards 2017-04-21 13:58:31 UTC
(In reply to Ivo Raisr from comment #1)
> Yes, indeed. Kernel takes care to copy TASK_COMM_LEN at max.
> Please could you supply a patch with the fix?

Thank you for the quick response.
 
I did look to see if I could find a quick fix.  I was looking for a
*asciiz function in memcheck/mc_main.c that could take a length limit
so that I could pass TASK_COMM_LEN to it.  I did not find one.
 
I thought it might be possible in coregrind/m_syswrap/syswrap-linux.c::PRE(sys_prctl) VKI_PR_SET_NAME case to change PRE_MEM_RASCIIZ to a strnlen call with TASK_COMM_LEN and then call PRE_MEM_READ with size.

I am not familiar with the valgrind codebase and I do not currently have time to work on a patch for this.
Comment 3 Ivo Raisr 2017-04-21 20:01:35 UTC
Alright, I will prepare a patch myself together with a fix for small problem in rt_sigsuspend syscall wrapper.
Comment 4 Peter (Stig) Edwards 2017-04-21 21:32:01 UTC
Created attachment 105142 [details]
A hack of a patch that just duplicates the asciiz func with a _limit_len version.

A hack of a patch that just duplicates the asciiz func with a _limit_len version.  A proper fix would avoid the duplication of code.
  Probably not that useful.
Comment 5 Ivo Raisr 2017-04-22 07:36:16 UTC
Created attachment 105145 [details]
patch utilizing VG_(strnlen) function
Comment 6 Ivo Raisr 2017-04-22 07:38:07 UTC
Based on your comment #2, I prepared a patch myself.
Compared to yours, it uses completely different approach.

I will defer to the community to judge which one suits better.
Comment 7 Peter (Stig) Edwards 2017-04-23 19:57:39 UTC
Thank you very much for the patch, I built and tested and it works as expected.

For the changes in coregrind/m_syswrap/syswrap-linux.c, I was thinking it might be better to use VKI_TASK_COMM_LEN ( as defined in include/vki/vki-linux.h ) and not 16.
Comment 8 Ivo Raisr 2017-04-24 08:52:47 UTC
Created attachment 105170 [details]
patch utilizing VG_(strnlen) function II.

Good point. I've modified my patch to use VKI_TASK_COMM_LEN.
Comment 9 Ivo Raisr 2017-04-24 21:31:19 UTC
Created attachment 105178 [details]
patch utilizing VG_(strnlen) and VG_(strlcpy) functions III.
Comment 10 Ivo Raisr 2017-04-24 21:32:43 UTC
Modified the previous patch slightly based on comment by Matthias Schwarzott. POST(sys_prctl) syscall wrapper needs to take into account that ARG2 might not need to be nul-terminated.
Comment 11 Ivo Raisr 2017-04-26 19:52:10 UTC
First part committed in SVN r16314.