Hi Alex,
Since this is searching for a file and not a directory, I didn't think
it was necessary, but I can see now that rstrstr may be better just in
case /bin/java appears somewhere in the middle of the path such as
~/bin/java16/bin/java.
thanks,
Chris
On 8/4/20 3:05 PM, Alex Menkov wrote:
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, [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
