bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

iraisr
Any comments or objections to patch v2 for bug 379039?
https://bugs.kde.org/show_bug.cgi?id=379039

I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

Matthias Schwarzott
Am 24.04.2017 um 17:00 schrieb Ivo Raisr:
> Any comments or objections to patch v2 for bug 379039?
> https://bugs.kde.org/show_bug.cgi?id=379039
>
> I.
>
Hi!

The code seems to work, but the len variable does not mean length of the
string, so it could be misleading.

Additionally I am not sure if the function POST(sys_prctl) must also be
modified.

The test memcheck/tests/threadname.c maybe needs more cases:

* Set threadname to a long string and check that only the first 15
characters are printed as threadname for the next error.

* If possible a test that proves, that POST(sys_prctl) does not access
memory after byte 16 (but I do not know how to test this).

Regards
Matthias


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

Matthias Schwarzott
Am 24.04.2017 um 22:03 schrieb Matthias Schwarzott:

> Am 24.04.2017 um 17:00 schrieb Ivo Raisr:
>> Any comments or objections to patch v2 for bug 379039?
>> https://bugs.kde.org/show_bug.cgi?id=379039
>>
>> I.
>>
> Hi!
>
> The code seems to work, but the len variable does not mean length of the
> string, so it could be misleading.
>
> Additionally I am not sure if the function POST(sys_prctl) must also be
> modified.
>
> The test memcheck/tests/threadname.c maybe needs more cases:
>
> * Set threadname to a long string and check that only the first 15
> characters are printed as threadname for the next error.

I forgot to mention that for this test prctl must be called directly
instead of pthread_setname_np as in the existing cases.
pthread_setname_np fails with ERANGE without calling prctl.

>
> * If possible a test that proves, that POST(sys_prctl) does not access
> memory after byte 16 (but I do not know how to test this).
>
> Regards
> Matthias
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Valgrind-developers mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

iraisr
In reply to this post by Matthias Schwarzott
2017-04-24 22:03 GMT+02:00 Matthias Schwarzott <[hidden email]>:

> Am 24.04.2017 um 17:00 schrieb Ivo Raisr:
>> Any comments or objections to patch v2 for bug 379039?
>> https://bugs.kde.org/show_bug.cgi?id=379039
>>
>> I.
>>
> Hi!
>
> The code seems to work, but the len variable does not mean length of the
> string, so it could be misleading.
>
> Additionally I am not sure if the function POST(sys_prctl) must also be
> modified.

Hi Matthias,

Thank you for your comments.
You are right, I had to modify POST(sys_prctl) so it takes into account
that ARG2 might not need to be nul-terminated.


> The test memcheck/tests/threadname.c maybe needs more cases:
>
> * Set threadname to a long string and check that only the first 15
> characters are printed as threadname for the next error.

Why? We do not want to do functional testing of libpthread or prctl syscall.

> * If possible a test that proves, that POST(sys_prctl) does not access
> memory after byte 16 (but I do not know how to test this).

That would be appropriate for prctl(get-name) case. Different story.

I will attach new patch shortly.
I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: bug 379039 - syscall wrapper for prctl(PR_SET_NAME)

Matthias Schwarzott
Am 24.04.2017 um 22:35 schrieb Ivo Raisr:

> 2017-04-24 22:03 GMT+02:00 Matthias Schwarzott <[hidden email]>:
>> Am 24.04.2017 um 17:00 schrieb Ivo Raisr:
>>> Any comments or objections to patch v2 for bug 379039?
>>> https://bugs.kde.org/show_bug.cgi?id=379039
>>>
>>> I.
>>>
>> Hi!
>>
>> The code seems to work, but the len variable does not mean length of the
>> string, so it could be misleading.
>>
>> Additionally I am not sure if the function POST(sys_prctl) must also be
>> modified.
>
> Hi Matthias,

Hi Ivo

>
> Thank you for your comments.
> You are right, I had to modify POST(sys_prctl) so it takes into account
> that ARG2 might not need to be nul-terminated.
>
Good. But not exactly matching what the kernel does.

This is from kernel/sys.c:

kernel/sys.c:   case PR_SET_NAME:
kernel/sys.c-           comm[sizeof(me->comm) - 1] = 0;
kernel/sys.c-           if (strncpy_from_user(comm, (char __user *)arg2,
kernel/sys.c-                                 sizeof(me->comm) - 1) < 0)
kernel/sys.c-                   return -EFAULT;
kernel/sys.c-           set_task_comm(me, comm);
kernel/sys.c-           proc_comm_connector(me);
kernel/sys.c-           break;

The kernel copies up to 15 characters from userspace. To mimic that
behaviour, I had to modify POST(sys_prctl) like this.

--- coregrind/m_syswrap/syswrap-linux.c
+++ coregrind/m_syswrap/syswrap-linux.c
@@ -1535,7 +1535,7 @@ POST(sys_prctl)
          const HChar* new_name = (const HChar*) ARG2;
          if (new_name) {    // Paranoia
             ThreadState* tst = VG_(get_ThreadState)(tid);
-            SizeT new_len = VG_(strnlen)(new_name, VKI_TASK_COMM_LEN);
+            SizeT new_len = VG_(strnlen)(new_name, VKI_TASK_COMM_LEN - 1);

             /* Don't bother reusing the memory. This is a rare event. */
             tst->thread_name =


>
>> The test memcheck/tests/threadname.c maybe needs more cases:
>>
>> * Set threadname to a long string and check that only the first 15
>> characters are printed as threadname for the next error.
>
> Why? We do not want to do functional testing of libpthread or prctl syscall.

The only reason is to see if valgrind can show the correct threadname in
error messages and I found the small behaviour difference above.

>
>> * If possible a test that proves, that POST(sys_prctl) does not access
>> memory after byte 16 (but I do not know how to test this).
>
> That would be appropriate for prctl(get-name) case. Different story.
>
No, I really meant if POST(sys_prctl) might crash when putting a
carefully crafted pointer to the syscall.
Maybe 16 bytes before the end of a memory page and having the next page
not mapped.

Regards
Matthias


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers