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

Alex Menkov alexey.menkov at oracle.com
Thu Aug 6 19:04:12 UTC 2020 

+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