+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
