RFR: JDK-8160997 Solaris: deprecated <pwd.h> and <gid.h> interfaces should be replaced

[Volker Simonis]

Hi Alan,

you write that "for Solaris 11, _POSIX_PTHREAD_SEMANTICS enables the
'official' POSIX" semantics. Is this strictly true only for Solaris 11
or also for Solaris 10 already. I know this question is not directly
related to your change which only applies to jdk9, but we may have to
downport this change to older Solaris releases so if you have any
experience with _POSIX_PTHREAD_SEMANTICS on Solaris 10 it would be
good to know.

Besides that, could you please also fix the following bits:

hotspot/src/os/solaris/vm/perfMemory_solaris.cpp
 - why do we need:

+#if defined(SOLARIS)

here. I think this file is always compiled with -DSOLARIS

jdk/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
jdk/src/jdk.security.auth/solaris/native/libjaas/Solaris.c
jdk/src/jdk.security.auth/unix/native/libjaas/Unix.c
- can you please use:
#if defined(__solaris__)
(as you did in ProcessHandleImpl_unix.c) instead of:
#if defined(SOLARIS)

In hotspot code it is more common to use our own platform defines from
the command line whereas in the jdk repository we usually use the
defualt system defines for the various platforms.

Thank you and best regards,
Volker



On Mon, Jul 11, 2016 at 6:16 PM, Alan Burlison <Alan.Burlison at oracle.com> wrote:
> On 11/07/2016 16:02, Daniel D. Daugherty wrote:
>
>> jdk/src/jdk.security.auth/solaris/native/libjaas/Solaris.c
>>      So you added the &p parameter to the getpwuid_r(), but
>>      then you never check or use the returned 'p'.
>>
>>      Other places that had both Solaris and non-Solaris variants
>>      had code like this:
>>
>>      L479:         if (result == 0 && p != NULL &&
>>      L480:             p->pw_name != NULL && *(p->pw_name) != '\0') {
>>      L481:             name = JNU_NewStringPlatform(env, p->pw_name);
>>      L482:         }
>>
>>      Of course, this Solaris specific code checked the returned
>>      '&pwd' value because it didn't have an &p parameter. I don't
>>      this is a problem, especially since I don't understand why
>>      this function has both an '&pwd' param and an '&p' param,
>>      but I figured I would raise the question.
>
>
> Ah, the difference between the POSIX draft & final versions isn't just a new
> parameter, they also changed the return type - ugh. I think the not
> null/empty check for pw_name is overkill but a closer look a the manpage
> suggests that the check for a non-zero result should also be accompanied by
> a check for a non-null value in p:
>
>        The getpwnam_r() and getpwuid_r() functions can return 0  in the
> case
>        where  the requested entry is not found. The application needs to
> check
>        the return value and that the result pointer is non-null. Otherwise,
> an
>        error value is returned to indicate the error.
>
> So I think the code in the patch should be:
>
>      if (getpwuid_r(getuid(), &pwd, pwd_buf, sizeof(pwd_buf), &p) != 0 && p
> != NULL &&
>          getgroups(numSuppGroups, groups) != -1) {
>
> The preceding memset() isn't necessary but does no harm.
>
> And while I'm in there I notice that the size of pw_buf is hardcoded, it
> should be whatever is returned by sysconf(_SC_GETPW_R_SIZE_MAX). That's
> actually 1024 but to be future-proof it should really use sysconf() - I'll
> fix that as well, and check the other files for the same sort of issue, and
> then redo the webrev and the tests.
>
>> jdk/src/jdk.security.auth/unix/native/libjaas/Unix.c
>>      No comments.
>>
>> Thumbs up! I don't know what to think of the libjaas/Solaris.c
>> code not using the possibly duplicate '&p' param.
>>>
>>>
>>> JPRT + the hotspot testset was clean.
>>>
>>
>> If I remember correctly, you also ran a regular JPRT test job
>> (without -testset hotspot). I believe that the regular job
>> runs JDK focused testing, but I'd have to look at the JPRT
>> job to be sure.
>
>
> I'll need to rerun them anyway after addressing the above issues, if there
> are other ones you'd like me to run then just let me know.
>
> Thanks,
>
> --
> Alan Burlison
> --