RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

[David Holmes]

Hi Serguei,

On 19/03/2013 2:47 AM, serguei.spitsyn at oracle.com wrote:
> I already reviewed this, and it is a good fix.
> Please, see my email attached.

You are not an OpenJDK Reviewer [1] . Staffan needs an official Review 
from a Reviewer before it can be pushed. That doesn't mean your review 
is not wanted or appreciated - it mostly certain is - but an official 
Review is still required.

Cheers,
David

[1] http://openjdk.java.net/bylaws#reviewer

> Thanks,
> Serguei
>
> On 3/18/13 7:13 AM, Staffan Larsen wrote:
>> Can I get an official Review of this change?
>>
>> Thanks,
>> /Staffan
>>
>> On 7 mar 2013, at 12:54, Staffan Larsen <staffan.larsen at oracle.com
>> <mailto:staffan.larsen at oracle.com>> wrote:
>>
>>> Here is a webrev for fixing this problem. I actually removed all of
>>> our own tokenization code in dll_build_name() and replaced it with
>>> calls to strtok_r (strtok_s on windows) instead. I think this should
>>> be more robust, at the cost of an extra memory allocation. I've also
>>> added the const qualifier to some of the parameters.
>>>
>>> http://cr.openjdk.java.net/~sla/8009558/webrev.02/
>>> <http://cr.openjdk.java.net/%7Esla/8009558/webrev.02/>
>>>
>>> All of the jdi and hprof tests passes with this change.
>>>
>>> Thanks,
>>> /Staffan
>>>
>>> On 6 mar 2013, at 20:48, serguei.spitsyn at oracle.com
>>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>>
>>>> Staffan,
>>>>
>>>> Thank you for the confirmation and taking care about this issue!
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 3/6/13 11:36 AM, Staffan Larsen wrote:
>>>>> Nice catch, Serguei!
>>>>>
>>>>> Unfortunately I have already pushed my change, but I filed
>>>>> "JDK-8009558: linked_md.c::dll_build_name can get stuck in an
>>>>> infinite loop" to track this new problem and I am working on a fix.
>>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>>
>>>>> On 5 mar 2013, at 20:26, serguei.spitsyn at oracle.com
>>>>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>>>>
>>>>>> Hi Staffan,
>>>>>>
>>>>>> Thank you for this discovery!
>>>>>> It looks good, but I have a couple of comments.
>>>>>>
>>>>>> It seems, there is one more problem in this code:
>>>>>>    68         /* check for NULL path */
>>>>>>    69         if (p == pathname) {
>>>>>>    70             continue;<== Endless loop if we hit this line
>>>>>>    71         }
>>>>>>
>>>>>> Do we need to do 'pathname++' before continuing at the line #70?
>>>>>> It is going to be endless loop in cases there is a PATH_SEPARATOR
>>>>>> at the beginning of paths or two PATH_SEPARATOR's in a row.
>>>>>> These would be incorrect path lists but the code above is
>>>>>> targeting exactly such cases.
>>>>>>
>>>>>> Also, the argument name "pathname" in the original code is confusing.
>>>>>> Should we rename it to "pathnames"?
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 3/4/13 10:56 AM, Staffan Larsen wrote:
>>>>>>> I accidentally stepped on this bug the other day. There is a
>>>>>>> problem in linker_md.c : dll_build_name() where an internal
>>>>>>> pointer can be moved to point outside the string. The code looks
>>>>>>> like:
>>>>>>>
>>>>>>>    57 static void dll_build_name(char* buffer, size_t buflen,
>>>>>>>    58                            const char* pname, const char*
>>>>>>> fname) {
>>>>>>>    59     // Based on os_solaris.cpp
>>>>>>>    60
>>>>>>>    61     char *path_sep = PATH_SEPARATOR;
>>>>>>>    62     char *pathname = (char *)pname;
>>>>>>>    63     while (strlen(pathname) > 0) {
>>>>>>>    64         char *p = strchr(pathname, *path_sep);
>>>>>>>    65         if (p == NULL) {
>>>>>>>    66             p = pathname + strlen(pathname);
>>>>>>>    67         }
>>>>>>>    68         /* check for NULL path */
>>>>>>>    69         if (p == pathname) {
>>>>>>>    70             continue;
>>>>>>>    71         }
>>>>>>>    72         (void)snprintf(buffer, buflen, "%.*s/lib%s."
>>>>>>> LIB_SUFFIX, (p - pathname),
>>>>>>>    73                        pathname, fname);
>>>>>>>    74
>>>>>>>    75         if (access(buffer, F_OK) == 0) {
>>>>>>>    76             break;
>>>>>>>    77         }
>>>>>>>    78         pathname = p + 1;
>>>>>>>    79         *buffer = '\0';
>>>>>>>    80     }
>>>>>>>
>>>>>>> If the supplied pname is a buffer with a simple path without any
>>>>>>> path separators in it, p will be set to the terminating nul on
>>>>>>> line 66. Then on line 78 it will be moved outside the buffer.
>>>>>>> Fixing this also necessitates fixes to the callers to check for
>>>>>>> an empty return string (in buffer).
>>>>>>>
>>>>>>> The same code show up in both the solaris code and the windows
>>>>>>> code as well as hprof.
>>>>>>>
>>>>>>> bug:http://bugs.sun.com/view_bug.do?bug_id=8009397
>>>>>>> webrev:http://cr.openjdk.java.net/~sla/8009397/webrev.00/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> /Staffan
>>>>>>
>>>>>
>>>>
>>>
>>
>