This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] [ping] nptl: Fix abort in case of set*id failure
- From: Florian Weimer <fweimer at redhat dot com>
- To: libc-alpha at sourceware dot org
- Date: Wed, 09 Jul 2014 12:42:56 +0200
- Subject: Re: [PATCH] [ping] nptl: Fix abort in case of set*id failure
- Authentication-results: sourceware.org; auth=none
- References: <20140428120545 dot 20B0643994596 at oldenburg dot str dot redhat dot com> <53B3F87E dot 5090507 at redhat dot com> <20140709060923 dot GN609 at spoyarek dot pnq dot redhat dot com>
On 07/09/2014 08:09 AM, Siddhesh Poyarekar wrote:
On Wed, Jul 02, 2014 at 02:18:06PM +0200, Florian Weimer wrote:
If a call to the set*id functions fails in a multi-threaded program,
the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
was triggered.
There should be a bug report for this I think. This is suitable for
2.20 since it is a regression from a patch that went in earlier to
abort on set*id failure.
I filed bug 17135 for this.
We address by checking that all calls to set*id on all threads give
the same result, and only abort if we see success followed by failure
(or vice versa).
The fix looks good to me barring a couple of minor issues that I have
mentioned below.
Thanks for your review.
The original bug report mentions a kernel failure case being the only
case causing the setuid failure, but I think there may be another case
worth mentioning. It goes like this:
1. A calls setuid(x) and starts signalling other threads
2. B is signalled with some other signal and its handler has a call to
setuid(y) - this is allowed since setuid is considered
async-signal-safe.
3. B signals A due to which A sets its uid to y
4. A tries calling setuid(x) on itself and fails.
3. and 4. may happen on any other thread too when they're setuid'd to
a uid that can't setuid to the other uid. This can result in
partially failed setuid calls for A as well as B.
Doesn't the stack_cache_lock in __nptl_setxid prevent this? I don't
know what it does to async-signal-safety, and there is likely some issue
here, but I think your A/B case is properly locked out.
@@ -1155,9 +1175,13 @@ __nptl_setxid (struct xid_command *cmdp)
cmdp->id[0], cmdp->id[1], cmdp->id[2]);
if (INTERNAL_SYSCALL_ERROR_P (result, err))
{
- __set_errno (INTERNAL_SYSCALL_ERRNO (result, err));
+ int error = INTERNAL_SYSCALL_ERRNO (result, err);
+ __nptl_setxid_error (cmdp, error);
+ __set_errno (error);
result = -1;
}
+ else
+ __nptl_setxid_error (cmdp, 0);
Get ERROR out and initialize it to 0 so that you can call
__nptl_setxid_error just once outside the if/else. Also, you could
mark the error path as unlikely. That is:
int error = 0;
if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
{
error = INTERNAL_SYSCALL_ERRNO (result, err);
__set_errno (error);
result = -1;
}
__nptl_setxid_error (cmdp, error);
Thanks for this suggestion, applied (twice).
--
Florian Weimer / Red Hat Product Security