Thanks!
On 8/6/20 12:04 PM, Alex Menkov wrote:
+1
--alex
On 08/06/2020 08:21, [email protected] 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, [email protected] 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, [email protected] wrote:
On 8/4/20 16:01, [email protected] 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 <[email protected]>
To: [email protected]
<[email protected]>, Alex Menkov
<[email protected]>, serviceability-dev
<[email protected]>
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, [email protected] 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, [email protected] 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
