[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(XS): 8248879: SA core file support on OSX has some bugs trying to locate the jvm libraries
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2020-07-29 3:40:31
Message-ID: 7fc9666b-ce1e-f83a-e7a7-e052ca451a09 () oracle ! com
[Download RAW message or body]

Hi Serguei and Alex,

Sorry about the delay getting back to this. I got sidetracked with other 
bugs and also realized the code needed more work than just Alex's 
suggestion for rstrstr().

As a bit of background first, get_real_path() is used to locate any 
library that is referenced from the core file using a relative path. So 
the core file will, for example, refer to @rpath/libjvm.dylib, and 
get_real_path() will convert that to a usable path to the file. Usually 
only JDK libraries and user libraries are specified with @rpath. System 
libraries all use full path names.

get_real_path() had a couple of shortcomings. The way it worked is if 
the specified execname ended in bin/java or if $JAVA_HOME was set, then 
it only checked for libraries in subdirs of the first one of those 2 
that it found to be valid. It would not look in both directories if both 
were valid, only in the first to be found valid. Only if neither of 
those were valid did it look in DYLD_LIBRARY_PATH. So, for example, as 
long as execname ended in bin/java, that's the only jdk directory that 
was checked for libraries. If it didn't end in bin/java, and $JAVA_HOME 
was set, then only it was checked. Then I added a 3rd option looking for 
the existence of any "bin/" in execname. Only if none of these 3 paths 
existed did the code defer to DYLD_LIBRARY_PATH. That made is hard to 
locate non JDK libraries, such as user JNI libraries, or to override the 
execname search for the JDK by setting $JAVA_HOME.

I've fixed this by having it check all 3 of the potential JDK locations 
not only to see if the paths are valid, but also if the library is in 
any of the paths, and then check all the paths DYLD_LIBRARY_PATH if it 
failed to find the library in the JDK paths. So now all the potential 
locations are checked to see if they contain the library. By doing this 
I was able to make it find the JDK libraries by properly specifying the 
execname or JAVA_HOME, and still find a user JNI library in 
DYLD_LIBRARY_PATH.

Since the code was kind of a mess and not well suited to just fix with 
some minor adjustments, I for the most part rewrote it. Although it 
still does a lot of the same things, it's much cleaner and easier to 
read now, and there's less replication of similar code. I also replaced 
strcat and strcpy calls with strncat and strncpy to prevent overflows. I 
would suggest for this review to just start by looking at 
get_real_path() and follow the code, and not compare the diffs, which 
aren't very readable.

http://cr.openjdk.java.net/~cjplummer/8248879/webrev.02/index.html

thanks,

Chris


On 7/14/20 8:54 PM, serguei.spitsyn@oracle.com wrote:
> Hi Alex,
>
> Yes, I understand this.
> After some thinking, I doubt my suggestion to check all occurrences or 
> "/bin/" is good. :)
>
> Thanks,
> Serguei
>
> On 7/14/20 18:19, Alex Menkov wrote:
>> Hi Serguei,
>>
>> On 07/14/2020 15:55, serguei.spitsyn@oracle.com wrote:
>>> Hi Chris and Alex,
>>>
>>> I agree the last occurrence of "/bin/" is better than the first.
>>> But I wonder if it makes sense to check all occurrences.
>>
>> The problem is strrstr (search for last occurrence) is not a part of 
>> std C lib.
>> So to avoid dependency on new library I suggested this simple 
>> implementation using standard strstr.
>>
>> --alex
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 7/14/20 15:14, Alex Menkov wrote:
>>>> Yes, you are right.
>>>> This is not a function from strings.h
>>>>
>>>> Ok, you can leave strstr (and keep in mind that the path can't 
>>>> contain "/bin/" other than jdk's bin) or implement the 
>>>> functionality. It should be something simple like
>>>>
>>>> static const char* rstrstr(const char *str, const char *sub) {
>>>>    const char *result = NULL;
>>>>    for (const char *p = strstr(str, sub); p != NULL; p = strstr(p + 
>>>> 1, sub)) {
>>>>        result = p;
>>>>    }
>>>>    return result;
>>>> }
>>>>
>>>> --alex
>>>>
>>>> On 07/14/2020 13:43, Chris Plummer wrote:
>>>>> Actually it's not so easy. I don't see any other references to 
>>>>> strrstr in our source. When I reference strstr, it gives a warning 
>>>>> because it's not declared. The only man page I can find says to 
>>>>> include sstring2.h, but this file does not exist. It also says to 
>>>>> link with -lsstrings2.
>>>>>
>>>>> Chris
>>>>>
>>>>> On 7/14/20 1:37 PM, Chris Plummer wrote:
>>>>>> Ok. I'll change both references to use strrstr.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 7/14/20 1:11 PM, Alex Menkov wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> I think it would be better to use strrstr to correctly handle 
>>>>>>> paths like
>>>>>>> /something/bin/jdk/bin/jhsdb
>>>>>>>
>>>>>>> And I'd updated
>>>>>>> 358     char* posbin = strstr(execname, "/bin/java");
>>>>>>> to use strrstr as well
>>>>>>>
>>>>>>> --alex
>>>>>>>
>>>>>>> On 07/14/2020 12:01, Chris Plummer wrote:
>>>>>>>> Ping!
>>>>>>>>
>>>>>>>> On 7/6/20 9:31 PM, Chris Plummer wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please help review the following:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8248879/webrev.00/index.html 
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8248879
>>>>>>>>>
>>>>>>>>> The description of the problem and the fix are both in the CR.
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>


[prev in list] [next in list] [prev in thread] [next in thread]