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

Chris Plummer chris.plummer at oracle.com
Thu Aug 6 20:06:26 UTC 2020 

Thanks!


On 8/6/20 12:04 PM, Alex Menkov wrote:
> +1
>
> --alex
>
> On 08/06/2020 08:21, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> Thank you for the update.
>> LGTM
>>
>> Thanks,
>> Serguei
>>
>>
>> On 8/5/20 12:47, Chris Plummer wrote:
>>> Hi Alex and Serguei,
>>>
>>> Here's an update. I think I covered all recommendations:
>>>
>>> http://cr.openjdk.java.net/~cjplummer/8248879/webrev.03/index.html
>>>
>>> Here's a diff of the changes since webrev.02:
>>>
>>> http://cr.openjdk.java.net/~cjplummer/8248879/webrev.03/ps_core.c.diff
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 8/4/20 4:59 PM, serguei.spitsyn at oracle.com wrote:
>>>> On 8/4/20 16:46, Chris Plummer wrote:
>>>>> On 8/4/20 4:41 PM, Chris Plummer wrote:
>>>>>> On 8/4/20 4:05 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> On 8/4/20 16:01, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> Just a quick comment.
>>>>>>>>
>>>>>>>> This fragment is not fully safe:
>>>>>>>> 356 strncpy(filepath, jdk_dir, BUF_SIZE - 1);
>>>>>>>> 357 strncat(filepath, jdk_subdir, BUF_SIZE -1);
>>>>>>>> 358 strncat(filepath, filename, BUF_SIZE - 1);
>>>>>>>> (The line 357 misses a space before 1.)
>>>>>>>>
>>>>>>>> Both strncpy and strncat define 'n' as max size of the 'src' 
>>>>>>>> string to be copied.
>>>>>>>> For instance, the strncat man says:
>>>>>>>>
>>>>>>>> *char *strncat(char **/dest/*, const char **/src/*, size_t* 
>>>>>>>> /n/*);*
>>>>>>>> "The *strncat*() function is similar, except that it will use 
>>>>>>>> at most /n/ bytes from /src/; ..."
>>>>>>>> Please, see: https://linux.die.net/man/3/strncat
>>>>>>> Forgot to say...
>>>>>>>
>>>>>>> This part of the strncpy description looks dangerous:
>>>>>>> If the length of /src/ is less than /n/, *strncpy*() *writes 
>>>>>>> additional null bytes to **/dest/**to ensure that a total of 
>>>>>>> **/n/**bytes are written*.
>>>>>>> See: https://linux.die.net/man/3/strncpy
>>>>>>>
>>>>>> Yes. The strncpy code is still correct since I only use it for 
>>>>>> the first copy, although it is a bit wasteful to have it add all 
>>>>>> those extra null bytes. I could probably get away with strcpy 
>>>>>> here since we know the incoming path are all limited in size to 
>>>>>> BUF_SIZE.
>>>>> I take that back. When JAVA_HOME is passed, there is nothing 
>>>>> preventing it from being any size, so it could be bigger than 
>>>>> BUF_SIZE. So I guess I need to leave it as strncpy.
>>>>
>>>> Then just keep in mind the 'filepath' in case of a size bigger that 
>>>> BUF_SIZE won't be null-terminated:
>>>> 356 strncpy(filepath, jdk_dir, BUF_SIZE - 1);
>>>> You may need to write the '\0' explicitly:
>>>> filepath[BUF_SIZE - 1] = '\0';
>>>> Alternatively, it can be simplier to initialize the local buffer:
>>>> 355 char filepath[BUF_SIZE] = { '\0' };
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> Chris
>>>>>>
>>>>>> Chris
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>> So, something like this would be safe:
>>>>>>>> 356 strncpy(filepath, jdk_dir, BUF_SIZE - 1);
>>>>>>>> 357 strncat(filepath, jdk_subdir, BUF_SIZE - 1 - 
>>>>>>>> strlen(filepath));
>>>>>>>> 358 strncat(filepath, filename, BUF_SIZE - 1 - strlen(filepath));
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/4/20 11:05, Chris Plummer wrote:
>>>>>>>>> Ping! Serguei and Alex can you have a look at this?
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> -------- Forwarded Message --------
>>>>>>>>> Subject:     Re: RFR(XS): 8248879: SA core file support on OSX 
>>>>>>>>> has some bugs trying to locate the jvm libraries
>>>>>>>>> Date:     Tue, 28 Jul 2020 20:40:31 -0700
>>>>>>>>> From:     Chris Plummer <chris.plummer at oracle.com>
>>>>>>>>> To:     serguei.spitsyn at oracle.com 
>>>>>>>>> <serguei.spitsyn at oracle.com>, Alex Menkov 
>>>>>>>>> <alexey.menkov at oracle.com>, serviceability-dev 
>>>>>>>>> <serviceability-dev at openjdk.java.net>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

More information about the serviceability-dev mailing list