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

[Alex Menkov]

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
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
> 
>