RFR(XS): 8248879: SA core file support on OSX has some bugs trying to locate the jvm libraries

Tue Aug 4 23:27:18 UTC 2020

Hi Alex,

Since this is searching for a file and not a directory, I didn't think 
it was necessary, but I can see now that rstrstr may be better just in 
case /bin/java appears somewhere in the middle of the path such as 
~/bin/java16/bin/java.

thanks,

Chris

On 8/4/20 3:05 PM, Alex Menkov wrote:
> Hi Chris,
>
> 396   posbin = strstr(execname, "/bin/java");
>
> I suppose this should be rstrstr.
>
>
> --alex
>
>
> On 07/28/2020 20:40, Chris Plummer wrote:
>> 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 at 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 at 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
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>>