Patchwork [libfortran] PR47375 GETLOG thread safety

login
register
mail settings
Submitter Janne Blomqvist
Date Jan. 24, 2011, 3:05 p.m.
Message ID <AANLkTinQU9K2HTSi6TgEgRmhVnNqW=cpb=PzaoqzB5yZ@mail.gmail.com>
Download mbox | patch
Permalink /patch/80176/
State New
Headers show

Comments

Janne Blomqvist - Jan. 24, 2011, 3:05 p.m.
Hello,

the attached patch makes the GETLOG intrinsic thread safe by using the
POSIX getpwuid_r() function, if available.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
Tobias Burnus - Jan. 25, 2011, 3:15 p.m.
Hi Janne,

On 01/24/2011 04:05 PM, Janne Blomqvist wrote:
> the attached patch makes the GETLOG intrinsic thread safe by using the
> POSIX getpwuid_r() function, if available.
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

The patch looks OK except for the following:

> @@ -75,7 +75,22 @@ PREFIX(getlog) (char * login, gfc_charlen_type login_len)
>
>     memset (login, ' ', login_len); /* Blank the string.  */
>   [...]
>     p_len = strlen (p);
>     if (login_len<  p_len)
> -    memcpy (login, p, login_len);
> -  else
> -    memcpy (login, p, p_len);
> +    p_len = login_len;
> +  memcpy (login, p, p_len);

The change looks wrong to me. I think it will write where it shouldn't 
for the following program (assuming that you do not have a len=1 login 
name):

character(len=1) :: str
call getlog(str)
print *, str
end

Tobias
Tobias Burnus - Jan. 25, 2011, 3:17 p.m.
On 01/25/2011 04:15 PM, Tobias Burnus wrote:
>>     p_len = strlen (p);
>>     if (login_len<  p_len)
>> -    memcpy (login, p, login_len);
>> -  else
>> -    memcpy (login, p, p_len);
>> +    p_len = login_len;
>> +  memcpy (login, p, p_len);
>
> The change looks wrong to me.

Scratch that - I saw one "-" too much. The patch is OK as is.

Tobias
Janne Blomqvist - Jan. 25, 2011, 4:49 p.m.
On Tue, Jan 25, 2011 at 17:17, Tobias Burnus <burnus@net-b.de> wrote:
> On 01/25/2011 04:15 PM, Tobias Burnus wrote:
>>>
>>>    p_len = strlen (p);
>>>    if (login_len<  p_len)
>>> -    memcpy (login, p, login_len);
>>> -  else
>>> -    memcpy (login, p, p_len);
>>> +    p_len = login_len;
>>> +  memcpy (login, p, p_len);
>>
>> The change looks wrong to me.
>
> Scratch that - I saw one "-" too much. The patch is OK as is.

Thanks,

Sending        libgfortran/ChangeLog
Sending        libgfortran/config.h.in
Sending        libgfortran/configure
Sending        libgfortran/configure.ac
Sending        libgfortran/intrinsics/getlog.c
Transmitting file data .....
Committed revision 169243.
Rainer Orth - Jan. 27, 2011, 2:41 p.m.
Janne Blomqvist <blomqvist.janne@gmail.com> writes:

> the attached patch makes the GETLOG intrinsic thread safe by using the
> POSIX getpwuid_r() function, if available.
>
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

This patch broke Solaris bootstrap:

/vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c: In function '_gfortran_getlog':
/vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c:89:3: error: too many arguments to function 'getpwuid_r'
/usr/include/pwd.h:191:23: note: declared here
/vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c:89:7: warning: assignment makes integer from pointer without a cast [enabled by default]

The problem is that <pwd.h> declares two different variants of
getpwuid_r:

#if (_POSIX_C_SOURCE - 0 >= 199506L) || defined(_POSIX_PTHREAD_SEMANTICS)
extern int getpwuid_r(uid_t, struct passwd *, char *, int, struct passwd **);
#else
extern struct passwd *getpwuid_r(uid_t, struct passwd *, char *, int);
#endif

There's this comment:

/*
 * getpwuid_r() & getpwnam_r() prototypes are defined here.
 */

/*
 * Previous releases of Solaris, starting at 2.3, provided definitions of
 * various functions as specified in POSIX.1c, Draft 6.  For some of these
 * functions, the final POSIX 1003.1c standard had a different number of
 * arguments and return values.
 *
 * The following segment of this header provides support for the standard
 * interfaces while supporting applications written under earlier
 * releases.  The application defines appropriate values of the feature
 * test macros _POSIX_C_SOURCE and _POSIX_PTHREAD_SEMANTICS to indicate
 * whether it was written to expect the Draft 6 or standard versions of
 * these interfaces, before including this header.  This header then
 * provides a mapping from the source version of the interface to an
 * appropriate binary interface.  Such mappings permit an application
 * to be built from libraries and objects which have mixed expectations
 * of the definitions of these functions.

The POSIX.1 definitions of getpwuid_r are only defined

#if (_POSIX_C_SOURCE - 0 >= 199506L) || defined(_POSIX_PTHREAD_SEMANTICS)

I'm not yet sure how and where best to handle this: perhaps we can do
this along the lines of what g++ defines (cf. gcc/config/sol2.h):

        /* For C++ we need to add some additional macro \
           definitions required by the C++ standard     \
           library.  */                                 \
        if (c_dialect_cxx ())                           \
          {                                             \
            builtin_define ("__STDC_VERSION__=199901L");\
            builtin_define ("_XOPEN_SOURCE=600");       \
            builtin_define ("_LARGEFILE_SOURCE=1");     \
            builtin_define ("_LARGEFILE64_SOURCE=1");   \
            builtin_define ("__EXTENSIONS__");          \
          }                                             \

For GNU/Linux, this is probably handled by -D_GNU_SOURCE in Makefile.am
(AM_CPPFLAGS), but this is obviously not general.

Alternatively, one could update the configure check to not only check
for the existance of getpwuid_r, but for a version that takes the
correct number and types of arguments.

	Rainer
Tobias Burnus - Jan. 27, 2011, 2:53 p.m.
On 01/27/2011 03:41 PM, Rainer Orth wrote:
>> the attached patch makes the GETLOG intrinsic thread safe by using the
>> POSIX getpwuid_r() function, if available.
> This patch broke Solaris bootstrap:
>
> The problem is that<pwd.h>  declares two different variants of
> getpwuid_r:

I have filled a tracking bug: PR 47491.

And I have a question to you, Rainer: There are similar patches for 
ctime_r and for ttyname_r. Do they work as is on Solaris or is also 
special care required?

See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and 
http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html

Tobias
Rainer Orth - Jan. 27, 2011, 3:05 p.m.
Hi Tobias,

> I have filled a tracking bug: PR 47491.

Thanks.

> And I have a question to you, Rainer: There are similar patches for ctime_r
> and for ttyname_r. Do they work as is on Solaris or is also special care
> required?
>
> See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and
> http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html

The only difference between the two ttyname_r variants is:

* full standard:

  extern int ttyname_r(int, char *, size_t);

* draft standard:

  extern char *ttyname_r(int, char *, int);

For ctime_r it's worse:

* full standard:

  extern char *ctime_r(const time_t *, char *);

* draft standards:

  extern char *ctime_r(const time_t *, char *, int);

So we should simply make sure that the standard versions are used.

	Rainer
Janne Blomqvist - Jan. 27, 2011, 5:20 p.m.
On Thu, Jan 27, 2011 at 17:05, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
> Hi Tobias,
>
>> I have filled a tracking bug: PR 47491.
>
> Thanks.
>
>> And I have a question to you, Rainer: There are similar patches for ctime_r
>> and for ttyname_r. Do they work as is on Solaris or is also special care
>> required?
>>
>> See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and
>> http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html
>
> The only difference between the two ttyname_r variants is:
>
> * full standard:
>
>  extern int ttyname_r(int, char *, size_t);
>
> * draft standard:
>
>  extern char *ttyname_r(int, char *, int);
>
> For ctime_r it's worse:
>
> * full standard:
>
>  extern char *ctime_r(const time_t *, char *);
>
> * draft standards:
>
>  extern char *ctime_r(const time_t *, char *, int);
>
> So we should simply make sure that the standard versions are used.

If my google-fu isn't failing me, it seems that the fix for these is
more or less the same as for the getpwuid_r() issue. That is, if my
currently posted patch adding AC_USE_SYSTEM_EXTENSIONS fixes the
getpwuid_r() issue, it should also fix ctime_r() and ttyname_r().
Rainer Orth - Jan. 27, 2011, 5:21 p.m.
Janne Blomqvist <blomqvist.janne@gmail.com> writes:

> If my google-fu isn't failing me, it seems that the fix for these is
> more or less the same as for the getpwuid_r() issue. That is, if my
> currently posted patch adding AC_USE_SYSTEM_EXTENSIONS fixes the
> getpwuid_r() issue, it should also fix ctime_r() and ttyname_r().

Exactly.
	Rainer

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 4f137e4..9b91f9a 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -249,7 +249,7 @@  AC_CHECK_FUNCS(chdir strerror getlogin gethostname kill link symlink perror)
 AC_CHECK_FUNCS(sleep time ttyname signal alarm ctime clock access fork execl)
 AC_CHECK_FUNCS(wait setmode execvp pipe dup2 close fdopen strcasestr getrlimit)
 AC_CHECK_FUNCS(gettimeofday stat fstat lstat getpwuid vsnprintf dup getcwd)
-AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r)
+AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r)
 
 # Check for glibc backtrace functions
 AC_CHECK_FUNCS(backtrace backtrace_symbols)
diff --git a/libgfortran/intrinsics/getlog.c b/libgfortran/intrinsics/getlog.c
index e75aa1c..baacdf0 100644
--- a/libgfortran/intrinsics/getlog.c
+++ b/libgfortran/intrinsics/getlog.c
@@ -1,8 +1,8 @@ 
 /* Implementation of the GETLOG g77 intrinsic.
-   Copyright (C) 2005, 2007, 2009 Free Software Foundation, Inc.
+   Copyright (C) 2005, 2007, 2009, 2011 Free Software Foundation, Inc.
    Contributed by François-Xavier Coudert <coudert@clipper.ens.fr>
 
-This file is part of the GNU Fortran 95 runtime library (libgfortran).
+This file is part of the GNU Fortran runtime library (libgfortran).
 
 Libgfortran is free software; you can redistribute it and/or
 modify it under the terms of the GNU General Public
@@ -75,7 +75,22 @@  PREFIX(getlog) (char * login, gfc_charlen_type login_len)
 
   memset (login, ' ', login_len); /* Blank the string.  */
 
-#if defined(HAVE_GETPWUID) && defined(HAVE_GETEUID)
+#if defined(HAVE_GETPWUID_R) && defined(HAVE_GETEUID)
+  struct passwd pwd;
+  struct passwd *result;
+  char *buf;
+  int err;
+  /* To be pedantic, buflen should be determined by
+     sysconf(_SC_GETPW_R_SIZE_MAX), which is 1024 on some tested
+     targets; we do something simple in case the target doesn't
+     support sysconf.  */
+  static const size_t buflen = 1024;
+  buf = get_mem (buflen);
+  err = getpwuid_r (geteuid (), &pwd, buf, buflen, &result);
+  if (err != 0 || result == NULL)
+    goto cleanup;
+  p = pwd.pw_name;
+#elif defined(HAVE_GETPWUID) && defined(HAVE_GETEUID)
   {
     struct passwd *pw = getpwuid (geteuid ());
     if (pw)
@@ -83,20 +98,22 @@  PREFIX(getlog) (char * login, gfc_charlen_type login_len)
     else
       return;
   }
-#else
-# ifdef HAVE_GETLOGIN
+#elif HAVE_GETLOGIN
   p = getlogin();
 # else
   return;
-# endif
 #endif
 
   if (p == NULL)
-    return;
+    goto cleanup;
 
   p_len = strlen (p);
   if (login_len < p_len)
-    memcpy (login, p, login_len);
-  else
-    memcpy (login, p, p_len);
+    p_len = login_len;
+  memcpy (login, p, p_len);
+
+ cleanup:
+#ifdef HAVE_GETPWUID_R
+  free (buf);
+#endif
 }