Bug 379039 - syscall wrapper for prctl(PR_SET_NAME) must not check more than 16 bytes
Summary: syscall wrapper for prctl(PR_SET_NAME) must not check more than 16 bytes
Status: ASSIGNED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.12.0
Platform: RedHat Enterprise Linux Linux
: NOR minor
Target Milestone: ---
Assignee: Ivo Raisr
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-21 12:55 UTC by Peter (Stig) Edwards
Modified: 2017-04-26 19:52 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
A hack of a patch that just duplicates the asciiz func with a _limit_len version. (6.94 KB, patch)
2017-04-21 21:32 UTC, Peter (Stig) Edwards
Details
patch utilizing VG_(strnlen) function (3.89 KB, patch)
2017-04-22 07:36 UTC, Ivo Raisr
Details
patch utilizing VG_(strnlen) function II. (164.08 KB, patch)
2017-04-24 08:52 UTC, Ivo Raisr
Details
patch utilizing VG_(strnlen) and VG_(strlcpy) functions III. (165.79 KB, patch)
2017-04-24 21:31 UTC, Ivo Raisr
Details

Note You need to log in before you can comment on or make changes to this bug.
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.