This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Linux: Implement per-thread user and group IDs
* Carlos O'Donell:
>> (Build-tested on all architectures with build-many-glibcs.py.
>> Run-time-tested on x86-64, by running the new xtest as root. I still
>> need to implement the inheritance test Carlos requested. Due to the
>> similarities, I plan to write one separate test for both kinds of
>> properties.)
>
> Right, the test for per-process is missing, we have only test for
> per-thread inheritance, and null attr default, but not per-process
> inheritance. I assume you'll fix it like you are going to fix the
> other-thread fs test case?
You've already seen the new separate test.
> The other thing I was thinking about is: Should a thread created
> with per-thread uid/gid block SIGSETXID permanently from startup?
> This way there can be no accident that the function sighandler_setxid
> is called? If you don't like that then at least some belt-and-suspenders
> code in sighandler_setxid to assert that a thread per-thread uid/gid
> will not have this handler ever called? Even better... do both! :-)
We might have to use SIGSETXID for something else in the future (or
perhaps we already do). I would leave this as it is today.
>> diff --git a/NEWS b/NEWS
>> index e63b69b930..83bd54b5e8 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -45,6 +45,12 @@ Major new features:
>> pthread_attr_setperthreadfs_np and pthread_attr_getperthreadfs_np have
>> been added in support of that.
>>
>> +* Likewise, on Linux, threads can now be created in such a way that they
>
> s/Likewise, on/On/g
There's a preceding NEWS entry with similar wording, so I thought the
“Likewise” would make sense here.
>
>> + retain per-thread user and group IDs. (This has always been the way how
>> + the Linux kernel implements threads.) The functions
>
> Drop the parenthetical.
I will do that.
>> +
>> +@deftypefun int pthread_attr_getperthreadid_np (pthread_attr_t
> *restrict @var{attr}, int *restrict @var{scope})
>
> Typo. s/pthread_attr_getperthreadid_np/pthread_attr_getperthreadids_np/g
Will fix.
>> +static const struct test_case test_cases[] =
>> + {
>> + { OP_NONE, { 0, }, { 0, 0, 0}, {0, 0, 0} },
>> +
>> + { OP_SETUID, { 1, }, { 1, 1, 1 }, { 0, } },
>> + { OP_SETEUID, { 2, }, { 0, 2, }, { 0, } },
>> + { OP_SETREUID, { 3, 4, }, { 3, 4, 4 }, { 0, } },
>> + { OP_SETRESUID, { 3, 4, 5 }, { 3, 4, 5 }, { 0, } },
>> +
>> + { OP_SETGID, { 6, }, { 0, }, { 6, 6, 6 } },
>> + { OP_SETEGID, { 7, }, { 0, }, { 0, 7, } },
>> + { OP_SETREGID, { 8, 9, }, { 0, }, { 8, 9, 9 } },
>> + { OP_SETRESGID, { 10, 11, 12 }, { 0, }, { 10, 11, 12 } },
>> +
>> + { OP_SETGROUPS, { -1, }, { 0, }, { 0, } },
>> + { OP_SETGROUPS, { 13, -1 }, { 0, }, { 0, } },
>> + { OP_SETGROUPS, { 13, 14, -1 }, { 0, }, { 0, } },
>> + { OP_SETGROUPS, { 13, 14, 15 }, { 0, }, { 0, } },
>> +
>> + /* Final round of checks. */
>> + { OP_NONE, { 0, }, { 0, }, {0, } },
>
> I find it more difficult to read and edit when elements of the
> struct are elided. Could you please fill them all in?
I can do this, although it will add quite a bit of cruft.
>> +/* An actual test thread. CLOSURE is a pointer to struct
>> + thread_argument, and perform the test according to this
>> + information. */
>> +static void *
>> +threadfunc (void *closure)
>> +{
>> + struct thread_argument *arg = closure;
>> +
>> + TEST_COMPARE (perthread_flag (pthread_self ()), arg->perthread);
>> +
>> + if (arg->indirect)
>> + {
>> + /* Only indirect once. */
>> + arg->indirect = false;
>> + /* Use the default attributes (NULL). This verifies that the
>> + per-thread scope is sticky. */
>> + pthread_t thr = xpthread_create (NULL, threadfunc, arg);
>> + TEST_COMPARE (perthread_flag (thr), arg->perthread);
>> + return xpthread_join (thr);
>> + }
>> +
>> + const struct test_case *expected = &test_cases[0];
>> + gid_t expected_supplementary[3] = { -1, -1, -1 };
>> + int expected_supplementary_count = 0;
>> +
>> + xpthread_barrier_wait (&barrier);
>
> All of these barrier waits should have notes about what they are
> synchronizing with, and which point if possible, thta way a future
> editor can know what they are wiating for. Perhaps numbering them
> with comments might help here.
>
> I realized late that I had the same problem while reviewing the
> other test case and it only occurred to me that we should just
> comment the phases.
Hmm. I'll see what I can do about this.
>> + /* Main thread and two other shared threads are extras. One
>> + per-thread test is skipped, so there are two extra threads. */
>
> ^^^ This comment needs clarification. Is it trying to rationalize why it's
> array_length (test_cases) + 2?
Yes, exactly. I will expand the comment.
>> + xpthread_barrier_init (&barrier, NULL, array_length (test_cases) + 2);
>> +
>> + pthread_t perthread_threads[array_length (test_cases)];
>> + /* Use thread index zero for no-op checking. */
>
> What is no-op checking?
It uses OP_NONE. In general, this is when the thread observes the UID
change from another thread and verifies that. I will try to make this
more explicit.
> This needs a block comment explaining what it is doing and why.
What about this?
/* Test both the direct and indirect case. Direct is when test
threads are launched with a per-thread attribute. Indirect is
when a thread is first launched with the a per-thread attribute,
but the actual test runs in another sub-thread, created with a
NULL attribute. */
>> + for (int indirect = 0; indirect < 2; ++indirect)
>> + {
>> + check_perthread (indirect);
>> +
/* Test two scenarios, one where set*id function is called from
the main thread, and other one where it is called from another
thread. */
>> + for (int broadcast_from_main = 0; broadcast_from_main < 2;
>> + ++broadcast_from_main)
/* Perform each of the test cases once, using a broadcast set*id
operation across multiple threads (those with per-process
scope). */
>> +/* Set *SCOPE to PTHREAD_PER_PROCESS_NP or PTHREAD_PER_THREAD_NP,
>> + depending on the state of *ATTR. */
>
> Wrong comment.
>
>> +int pthread_attr_getperthreadids_np (const pthread_attr_t *__restrict __attr,
>> + int *__restrict __scope)
>> + __THROW __nonnull ((1, 2));
>> +
Sorry, wrong comment in what way?