From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62794 invoked by alias); 27 Jun 2017 11:10:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 62776 invoked by uid 89); 27 Jun 2017 11:10:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 27 Jun 2017 11:10:30 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-MBX-04.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1dPoO4-0003PT-3q from Tom_deVries@mentor.com ; Tue, 27 Jun 2017 04:10:28 -0700 Received: from [127.0.0.1] (137.202.0.87) by SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) with Microsoft SMTP Server (TLS) id 15.0.1263.5; Tue, 27 Jun 2017 12:10:22 +0100 Subject: Re: [PATCH] Use secure_getenv for GOMP_DEBUG To: Jakub Jelinek CC: Joseph Myers , GCC Patches , Thomas Schwinge References: <2413b0f6-9cb2-243f-d805-08323a9c9a0a@mentor.com> <1de94c83-96da-f380-9964-1472f63270c9@mentor.com> <20170626152911.GK2123@tucnak> <9110a6a9-43dc-52ca-aabf-20c73ac73775@mentor.com> <20170627073805.GP2123@tucnak> From: Tom de Vries Message-ID: <46104e74-3bac-13b1-54ca-025eb5c48920@mentor.com> Date: Tue, 27 Jun 2017 11:10:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170627073805.GP2123@tucnak> Content-Type: multipart/mixed; boundary="------------6B1BC741BB3BA0AB1DF10143" X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) X-SW-Source: 2017-06/txt/msg02009.txt.bz2 --------------6B1BC741BB3BA0AB1DF10143 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3642 On 06/27/2017 09:38 AM, Jakub Jelinek wrote: > On Tue, Jun 27, 2017 at 09:17:57AM +0200, Tom de Vries wrote: >> This patch uses secure_getenv for GOMP_DEBUG. >> >> It factors out the secure_getenv code from plugin-hsa.c into >> libgomp/secure_getenv.h, and reuses it in env.c. >> >> I've added _GNU_SOURCE before the libgomp.h include in env.c to make sure >> that secure_getenv (imported from stdlib.h) is available. >> >> I've also added a test-case that sets GOMP_DEBUG to 1 and verifies that some >> output is generated. >> >> Build for c-only on x86_64 without accelerator, tested libgomp -m64/-m32. >> >> OK if x86_64 bootstrap and reg-test succeeds? >> >> Thanks, >> - Tom > >> Use secure_getenv for GOMP_DEBUG >> > >> --- /dev/null >> +++ b/libgomp/secure_getenv.h >> @@ -0,0 +1,53 @@ >> +/* Copyright (C) 2017 Free Software Foundation, Inc. >> + >> +This file is part of GCC. >> + >> +GCC is free software; you can redistribute it and/or modify >> +it under the terms of the GNU General Public License as published by >> +the Free Software Foundation; either version 3, or (at your option) >> +any later version. >> + >> +GCC is distributed in the hope that it will be useful, >> +but WITHOUT ANY WARRANTY; without even the implied warranty of >> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +GNU General Public License for more details. >> + >> +Under Section 7 of GPL version 3, you are granted additional >> +permissions described in the GCC Runtime Library Exception, version >> +3.1, as published by the Free Software Foundation. >> + >> +You should have received a copy of the GNU General Public License and >> +a copy of the GCC Runtime Library Exception along with this program; >> +see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >> +. */ >> + >> +#ifndef _SECURE_GETENV_H >> +#define _SECURE_GETENV_H 1 >> + >> +/* Secure getenv() which returns NULL if running as SUID/SGID. */ >> +#ifndef HAVE_SECURE_GETENV >> +#ifdef HAVE___SECURE_GETENV >> +#define secure_getenv __secure_getenv >> +#elif defined (HAVE_UNISTD_H) && defined(HAVE_GETUID) && defined(HAVE_GETEUID) \ >> + && defined(HAVE_GETGID) && defined(HAVE_GETEGID) >> + >> +#include >> + >> +/* Implementation of secure_getenv() for targets where it is not provided but >> + we have at least means to test real and effective IDs. */ >> + >> +static char * >> +secure_getenv (const char *name) > > Shouldn't this be static inline char * ? I mean, even at -O0 we don't want > it to be emitted into every TU. > Done. > Another thing is that we probably want to follow what libgfortran does for > the case where secure_getenv isn't available, but __secure_getenv is - > in particular emit this function (if geteuid and getegid are present), but > emit a weakref call to __secure_getenv first if non-NULL. > I've copied the approach from libgfortran. Build and tested: - once as is, and - once with '#ifndef HAVE_SECURE_GETENV' replaced with '#if 1' to trigger the fallback. >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/gomp-debug-env.c >> @@ -0,0 +1,13 @@ >> +/* { dg-do run } */ >> +/* { dg-set-target-env-var GOMP_DEBUG "1" } */ >> + >> +/* Check that GOMP_DEBUG=1 triggers some output. */ >> + >> +int >> +main (void) >> +{ >> +#pragma acc parallel >> + ; >> +} >> + >> +/* { dg-output "GOACC_parallel_keyed" } */ > > Does dg-set-target-env-var work for remote testing? No, it'll make the testcase unsupported for remote testing. [ Discussed before at f.i. https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01808.html ] Thanks, - Tom --------------6B1BC741BB3BA0AB1DF10143 Content-Type: text/x-patch; name="0001-Use-secure_getenv-for-GOMP_DEBUG.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0001-Use-secure_getenv-for-GOMP_DEBUG.patch" Content-length: 7667 Use secure_getenv for GOMP_DEBUG 2017-06-26 Tom de Vries * env.c (parse_unsigned_long_1): Factor out of ... (parse_unsigned_long): ... here. (parse_int_1): Factor out of ... (parse_int): ... here. (parse_int_secure): New function. (initialize_env): Use parse_int_secure for GOMP_DEBUG. * secure_getenv.h: Factor out of ... * plugin/plugin-hsa.c: ... here. * testsuite/libgomp.oacc-c-c++-common/gomp-debug-env.c: New test. --- libgomp/env.c | 44 +++++++++++++--- libgomp/plugin/plugin-hsa.c | 27 +--------- libgomp/secure_getenv.h | 60 ++++++++++++++++++++++ .../libgomp.oacc-c-c++-common/gomp-debug-env.c | 13 +++++ 4 files changed, 111 insertions(+), 33 deletions(-) diff --git a/libgomp/env.c b/libgomp/env.c index ced752d..802c73b 100644 --- a/libgomp/env.c +++ b/libgomp/env.c @@ -26,6 +26,7 @@ /* This file defines the OpenMP internal control variables and arranges for them to be initialized from environment variables at startup. */ +#define _GNU_SOURCE #include "libgomp.h" #include "gomp-constants.h" #include @@ -58,6 +59,8 @@ #endif #endif /* LIBGOMP_OFFLOADED_ONLY */ +#include "secure_getenv.h" + struct gomp_task_icv gomp_global_icv = { .nthreads_var = 1, .thread_limit_var = UINT_MAX, @@ -171,15 +174,17 @@ parse_schedule (void) } /* Parse an unsigned long environment variable. Return true if one was - present and it was successfully parsed. */ + present and it was successfully parsed. If SECURE, use secure_getenv to the + environment variable. */ static bool -parse_unsigned_long (const char *name, unsigned long *pvalue, bool allow_zero) +parse_unsigned_long_1 (const char *name, unsigned long *pvalue, bool allow_zero, + bool secure) { char *env, *end; unsigned long value; - env = getenv (name); + env = (secure ? secure_getenv (name) : getenv (name)); if (env == NULL) return false; @@ -206,14 +211,23 @@ parse_unsigned_long (const char *name, unsigned long *pvalue, bool allow_zero) return false; } +/* As parse_unsigned_long_1, but always use getenv. */ + +static bool +parse_unsigned_long (const char *name, unsigned long *pvalue, bool allow_zero) +{ + return parse_unsigned_long_1 (name, pvalue, allow_zero, false); +} + /* Parse a positive int environment variable. Return true if one was - present and it was successfully parsed. */ + present and it was successfully parsed. If SECURE, use secure_getenv to the + environment variable. */ static bool -parse_int (const char *name, int *pvalue, bool allow_zero) +parse_int_1 (const char *name, int *pvalue, bool allow_zero, bool secure) { unsigned long value; - if (!parse_unsigned_long (name, &value, allow_zero)) + if (!parse_unsigned_long_1 (name, &value, allow_zero, secure)) return false; if (value > INT_MAX) { @@ -224,6 +238,22 @@ parse_int (const char *name, int *pvalue, bool allow_zero) return true; } +/* As parse_int_1, but use getenv. */ + +static bool +parse_int (const char *name, int *pvalue, bool allow_zero) +{ + return parse_int_1 (name, pvalue, allow_zero, false); +} + +/* As parse_int_1, but use getenv_secure. */ + +static bool +parse_int_secure (const char *name, int *pvalue, bool allow_zero) +{ + return parse_int_1 (name, pvalue, allow_zero, true); +} + /* Parse an unsigned long list environment variable. Return true if one was present and it was successfully parsed. */ @@ -1207,7 +1237,7 @@ initialize_env (void) gomp_global_icv.thread_limit_var = thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var; } - parse_int ("GOMP_DEBUG", &gomp_debug_var, true); + parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true); #ifndef HAVE_SYNC_BUILTINS gomp_mutex_init (&gomp_managed_threads_lock); #endif diff --git a/libgomp/plugin/plugin-hsa.c b/libgomp/plugin/plugin-hsa.c index 90ca247..adb07ac 100644 --- a/libgomp/plugin/plugin-hsa.c +++ b/libgomp/plugin/plugin-hsa.c @@ -39,32 +39,7 @@ #include #include "libgomp-plugin.h" #include "gomp-constants.h" - -/* Secure getenv() which returns NULL if running as SUID/SGID. */ -#ifndef HAVE_SECURE_GETENV -#ifdef HAVE___SECURE_GETENV -#define secure_getenv __secure_getenv -#elif defined (HAVE_UNISTD_H) && defined(HAVE_GETUID) && defined(HAVE_GETEUID) \ - && defined(HAVE_GETGID) && defined(HAVE_GETEGID) - -#include - -/* Implementation of secure_getenv() for targets where it is not provided but - we have at least means to test real and effective IDs. */ - -static char * -secure_getenv (const char *name) -{ - if ((getuid () == geteuid ()) && (getgid () == getegid ())) - return getenv (name); - else - return NULL; -} - -#else -#define secure_getenv getenv -#endif -#endif +#include "secure-getenv.h" /* As an HSA runtime is dlopened, following structure defines function pointers utilized by the HSA plug-in. */ diff --git a/libgomp/secure_getenv.h b/libgomp/secure_getenv.h new file mode 100644 index 0000000..1d03eb7 --- /dev/null +++ b/libgomp/secure_getenv.h @@ -0,0 +1,60 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation; either version 3, or (at your option) +any later version. + +GCC is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +Under Section 7 of GPL version 3, you are granted additional +permissions described in the GCC Runtime Library Exception, version +3.1, as published by the Free Software Foundation. + +You should have received a copy of the GNU General Public License and +a copy of the GCC Runtime Library Exception along with this program; +see the files COPYING3 and COPYING.RUNTIME respectively. If not, see +. */ + +#ifndef _SECURE_GETENV_H +#define _SECURE_GETENV_H 1 + +/* Secure getenv() which returns NULL if running as SUID/SGID. */ +#ifndef HAVE_SECURE_GETENV +#if defined (HAVE_UNISTD_H) && defined(HAVE_GETUID) && defined(HAVE_GETEUID) \ + && defined(HAVE_GETGID) && defined(HAVE_GETEGID) + +#include + +#if SUPPORTS_WEAKREF && defined(HAVE___SECURE_GETENV) +static char* weak_secure_getenv (const char*) + __attribute__((__weakref__("__secure_getenv"))); +#endif + +/* Implementation of secure_getenv() for targets where it is not provided but + we have at least means to test real and effective IDs. */ + +static inline char * +secure_getenv (const char *name) +{ +#if SUPPORTS_WEAKREF && defined(HAVE___SECURE_GETENV) + if (weak_secure_getenv) + return weak_secure_getenv (name); +#endif + + if ((getuid () == geteuid ()) && (getgid () == getegid ())) + return getenv (name); + else + return NULL; +} +#else +#define secure_getenv getenv +#endif +#endif + +#endif /* _SECURE_GETENV_H. */ diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/gomp-debug-env.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/gomp-debug-env.c new file mode 100644 index 0000000..3fc3503 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/gomp-debug-env.c @@ -0,0 +1,13 @@ +/* { dg-do run } */ +/* { dg-set-target-env-var GOMP_DEBUG "1" } */ + +/* Check that GOMP_DEBUG=1 triggers some output. */ + +int +main (void) +{ +#pragma acc parallel + ; +} + +/* { dg-output "GOACC_parallel_keyed" } */ --------------6B1BC741BB3BA0AB1DF10143--