No agent #1245
No agent #1245
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1245 +/- ##
==========================================
- Coverage 87.19% 86.89% -0.31%
==========================================
Files 113 113
Lines 10296 10336 +40
Branches 4051 4065 +14
==========================================
+ Hits 8978 8981 +3
- Misses 726 760 +34
- Partials 592 595 +3
|
|
@tachyonicClock I am taking another crack at the non-ascii paths. To deal with them I am setting up a temporary directory and just copying the required jar file in place. I will then try to bootstrap the rest. Thus far everything but services are working. |
|
I believe using a temporary directory is likely the most effective workaround. Linking vs. Copying: If possible, we should try to hard link the libraries rather than copying them. This will require a fallback for people linking between disk drives, since you cannot hard-link across storage devices. Since you can symbolic link directories across drives we could try that but it might get messy. Symbolic linking or hard-linking will add close to zero startup time. Uniform Behavior: Branching on Custom Loaders: For people who need custom loaders or things on the class path at startup, we could expose a more flexible startJVM function:
|
|
The previous fix that included the agent was mis-behaving because dlls from a different jdk vendor were being loaded with the specified jvm.dll. Why this issue never appeared in the past, I'm not so sure. It was basically a dll search path issue. Also, symlinks on windows requires admin. |
|
@tachyonicClock I agree that uniform behavior would be best. I would make everything go through our system class loader if I through that we could get away with it. However, there is a warning that is making me nervous... OpenJDK 64-Bit Server VM warning: Archived non-system classes are disabled because the java.system.class.loader property is specified (value = "org.jpype.classloader.DynamicClassLoader"). To use archived non-system classes, this property must not be set I have no idea what a Archived non-system class would be or what they are used for. NationalSecurityAgency/ghidra#2400 As to linking that is really only a unix thing. For now I will copy. I may be able to unlink it after starting but there are risks with that. More experiments needed. I copied my whole jpype install under a ./unicode_à😎 directory and trying to get it all working there. Still a bunch of edge cases. But if we can get all the tests running there than we should be set. |
|
Still running into edge cases here. Down to two issues.
|
|
|
||
| private static URL[] launch() | ||
| { | ||
| String cp = System.getProperty("jpype.class.path"); | ||
| if (cp == null) | ||
| return new URL[0]; | ||
|
|
||
| ArrayList<URL> path = new ArrayList<>(); | ||
| int off = 0, next; | ||
| do | ||
| { | ||
| next = cp.indexOf(File.pathSeparator, off); | ||
| String element = (next == -1) | ||
| ? cp.substring(off) | ||
| : cp.substring(off, next); | ||
| if (!element.isEmpty()) | ||
| { | ||
| try | ||
| { | ||
| URL url = Paths.get(element).toUri().toURL(); | ||
| if (url != null) | ||
| path.add(url); | ||
| } catch (MalformedURLException ex) | ||
| { | ||
| System.err.println("Malformed url "+ element); | ||
| } catch (IOException ex) | ||
| { | ||
| System.err.println("Unable to open "+ element); | ||
| } | ||
| } | ||
| off = next + 1; | ||
| } while (next != -1); | ||
|
|
||
| System.out.println("jpype.class.path " + cp); | ||
| System.clearProperty("jpype.class.path"); | ||
| System.setProperty("java.class.path", cp); | ||
| return path.toArray(new URL[0]); | ||
| } |
There was a problem hiding this comment.
@tachyonicClock This section allows for us to load the non-ascii path just like a normal one. Thus removing the need for late load logic.
|
I understand the two issues. The key problem is that callerSensitive picks up the class loader for the class which was loaded not the one called from Python. So as org.jpype is loaded from the system classloader it gets the broken ApplicationClassLoader rather than our system specified class loader. As usually there are clear gaps in the Java api which are driving this. Only in the case that ApplicationClassLoader was use can this work perfectly. I can make dbapi2 work if I force the caller sensitive logic off, but I am not sure if that works in all cases. |
|
So close but still no cigar on this one. Each fix has its own downsides and each is thwarted by something deep in the Java API. I am guessing next attack will be to make a separate org.jpype.cl jar file with just the dynamic class loader. The issue all stem from our caller sensitive grabbing the broken application loader. Thus I need to fool it into thinking we are calling something loaded from the DynamicClassLoader level. Other than splitting the jars not much chance making such flip. |
Unfortunately, using a temporary directory will cause it to always fail for anyone using windows with non ascii characters in their username. |
|
There is one unresolved issue which I don't know how to test in the matrix. If the Python shared library is on a non-ascii path, then I get this very inscrutable error. |
|
Love when they have an error for ages but they won't fix it because it would break compatibility with being broken. |
|
I don't believe it is possible to fix the issue of the dll located on a non-ascii path on windows. The problem stems from this code in Java. They are calling the ascii only version of LoadLibrary. Apparently the correct solution is to convert to wide characters first than call LoadLibraryW. Unfortunately, this is above my level to deal with. The only work around I see:
The second is possible, but would require a special script just to maintain it. The first I don't know how to start with because I am not sure how windows code pages even appear in Java. |
|
@tachyonicClock I found the solution to ascii characters on the path, though it is horrible. Apparently when you convert the path name to Short it will give you a path which works regardless. I hacked that into the process. The last part is intractable. Whenever you run JPype on a Windows machine with a non-ascii path for the install directory, it is going to need to make a temporary copy of the jar. BUT it is also going to hold that file handle open forever with no way of deleting it from within the process that I have found. I put in a good 4 hours trying different work arounds. I have no clue why Java keeps the file open on Windows and not on Linux. We are technically working in that all of the functionality is present. We need help from OpenJDK to implement better support for this problem. I have done all that is feasible. Though perhaps someone else as a creative idea. |
|
@marscher This is ready for a review, but contains enough black magic that a horcrux may have been created. There were at least 5 separate bugs in the JVM that I was trying to dodge.
|
https://docs.oracle.com/javase/8/docs/api/java/net/URLClassLoader.html#close-- You might just need to override close to close all the other ones and/or manually close it yourself before shutting down. |
|
The issue isn't the ones that we load. The issue is that on the system class loader itself. It is loaded by init level 3 and we will need to delete it at the end. Because it is the second item loaded in the system (before we have that ability to call a register native or insert class) it occupies a special place. I will look if perhaps there is a way to trash it on the shutdown sequence, but I suspect that it may be aone and done start given its location. |
|
@marscher I am hoping to get 1.5.2 out in the next few weeks. Will you have time to review or should I try to find others to assist? |
I was getting ready to upgrade Ghidra to JPype 1.5.1 when the mismatching java versions problem popped up. We plan on code freezing by the end of the month, so I'm debating if I should hold off for 1.5.2 and keep us at 1.5.0 which means we won't support Python 3.13, or upgrade to 1.5.1 knowing that 1.5.2 might not be out in time. If i do the latter, i'd like to at least document the known limitations, like the JDK on the PATH needing to match the version that gets passed to JPype. Maybe this choice will be made moot if 1.5.2 comes out relatively soon. |
|
Assuming the code is ready I will be releasing this weekend or next. We don't like leaving broken versions out for long. I do think documention of the weird platforms out there may still be necessary. Given nice, complete,well installed versions are on the test system. And users often have broken, incomplete, mismatched arch versions. We have no way to know that 1.5.2 won't also be a dud unless I give up on the non-ascii jar path. Even then once I try to support modules, once again we may produce another dud. Thus unless you are shipping all the pieces there will always be risk. That doesn't mean we won't do our best to support the majority of installations, but we are only human. A number of things are broken in openJDK and have been for 10 years or more. Much of that came about because things were broken in windows and the Java developers threw up their hands. Thus we are fighting on a very shaky foundation. |
|
Sounds good. We also see every possible combination of JDK's and platforms, so Ghidra will likely be a good real-world testing ground. |
|
@ryanmkurtz Do you have an opinion on dropping 1.8? We have been holding out for a long time, but much of what I want to do going forward is going to be module based. Thus I have a need to drop it. |
|
We have been switching to the latest LTS release about 6-12 months after it's released, so we are currently on JDK 21. Dropping 1.8 def. won't affect us or our users. |
|
@marscher Any chance you can start the reviews this week? |
|
@astrelsky would you mind reviewing this code instead? I can't leave a defective version out there and have to start the ball on a replacement. But if I don't cut a new release soon I may end up in another busy period at work with little time to address issues. |
I can. I probably won't get to it until tomorrow night. |
There was a problem hiding this comment.
I'll see if I can break it tomorrow afternoon, as I'm burnt out for the night, but this looks good to me.
I forgot to check and am on my phone now. Is the case where the copied jar to the temp directory already exists handled? This could occur with multiprocessing before jvm startup maybe. I'm not sure if the temp path name used is randomized.
native/common/jp_context.cpp
Outdated
| char shortPathBuffer[MAX_PATH]; | ||
| GetShortPathName(path, shortPathBuffer, MAX_PATH); | ||
| return shortPathBuffer; |
There was a problem hiding this comment.
This had me do a double take for a moment because it appears to be returning an array on the stack even though it is not. Not that there is any better alternative.
According to the documentation for GetShortPathName, this can fail. Some file systems don't support the short name. Additionally, the documentation says that this can succeed but still return the long name instead, which is fun.
Is the UNICODE macro undefined or explicitly not set somewhere so it doesn't pick the wide char version?
There was a problem hiding this comment.
The issue here was that GetShortPathName ate the non-ascii characters which allows java to load the library properly. This was the only pattern that I found to work consistently. UNICODE doesn't help the problem as the issue is that Java doesn't accept non-ascii anywhere in the path. It is better to let it mangle to "a1/b2/org.jpype.lib" which somehow allows it to function properly.
There was a problem hiding this comment.
Ok. GetShortPathName is a macro though. If UNICODE is defined it will expand to GetShortPathNameW otherwise GetShortPathNameA.
I understand why this is being done. My point is that it can fail. As long as it fails in a way that we'll know what happened when the user reports a problem, then it's fine.
There was a problem hiding this comment.
All good. I let it fall through to the old behavior.
| if (dladdr((void*)getShared, &info)) | ||
| return info.dli_fname; |
There was a problem hiding this comment.
This seems fine, I can't think of anything that would cause it to break.
| // is holding the file open and there is no apparent method to close it | ||
| // so that this can succeed | ||
| if (jarTmpPath != "") | ||
| remove(jarTmpPath.c_str()); |
There was a problem hiding this comment.
You need to use URLClassLoader.close on the SystemClassLoader. Once closed it will be unable to load anymore classes which have not yet been loaded. Since we are shutting down here, it will be fine, you just need to call close before shutdownJVM (obviously). This won't have any effect on standard Java classes since they are in the builtin and platform class loaders.
There was a problem hiding this comment.
I called "close" on the JPypeClassLoader which is the system classloader. Unfortunately, that didn't change anything.
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\nelson85\\AppData\\Local\\Temp\\tmpfrr4g750'
The problem is at the ApplicationClassLoader which is not a URL classloader.
private static class AppClassLoader extends BuiltinClassLoader {
static {
if (!ClassLoader.registerAsParallelCapable())
throw new InternalError();
}
AppClassLoader(BuiltinClassLoader parent, URLClassPath ucp) {
super("app", parent, ucp);
}
@Override
protected Class<?> loadClass(String cn, boolean resolve)
throws ClassNotFoundException
{
// for compatibility reasons, say where restricted package list has
// been updated to list API packages in the unnamed module.
@SuppressWarnings("removal")
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
int i = cn.lastIndexOf('.');
if (i != -1) {
sm.checkPackageAccess(cn.substring(0, i));
}
}
return super.loadClass(cn, resolve);
}
@Override
protected PermissionCollection getPermissions(CodeSource cs) {
PermissionCollection perms = super.getPermissions(cs);
perms.add(new RuntimePermission("exitVM"));
return perms;
}
/**
* Called by the VM to support dynamic additions to the class path
*
* @see java.lang.instrument.Instrumentation#appendToSystemClassLoaderSearch
*/
void appendToClassPathForInstrumentation(String path) {
appendClassPath(path);
}
/**
* Called by the VM to support define package for AppCDS
*/
protected Package defineOrCheckPackage(String pn, Manifest man, URL url) {
return super.defineOrCheckPackage(pn, man, url);
}
/**
* Called by the VM, during -Xshare:dump
*/
private void resetArchivedStates() {
setClassPath(null);
}
}
I can't find a close method in any of the source code or documentation. I would hope there would be a way to close or at least remove the lock.
There was a problem hiding this comment.
Ah for some reason I was thinking that this was for our system ClassLoader. Nevermind.
| jvmargs = _removeClassPath(jvmargs) | ||
| late_load = False | ||
| elif has_classloader: | ||
| # Code for 1.6 release when we add module support |
There was a problem hiding this comment.
This is getting very complicated. I contemplated it a few times with the previous changes but it may be a good idea to split it up more when adding the module path handling later.
There was a problem hiding this comment.
Do you see a way to clear this up or can we wait for the module path patch first?
There was a problem hiding this comment.
I would just wait for the module patch.
|
Disregard that, it looks like I had an issue where a free-threaded build was laying around and loaded instead. |
|
Ok good news I can't break it and #1242 is not necessary. |
|
Okay I will work on making changes today and see if we can get a release candidate ready. I may need to ask others to test. Our testing matrix really can't cover all the deployments out there. Especially those partial jvms and international installs. |
| @@ -49,7 +49,8 @@ | |||
| # 3.8 onward | |||
| from typing import Protocol, runtime_checkable | |||
| from typing import SupportsIndex, SupportsFloat # lgtm [py/unused-import] | |||
| from typing import Sequence, Mapping, Set, Callable # lgtm [py/unused-import] | |||
| # lgtm [py/unused-import] | |||
| from typing import Sequence, Mapping, Set, Callable | |||
Check notice
Code scanning / CodeQL
Unused import Note
This resolves the issued with the agent. But I would like to fix the non-ascii path issue at the same time. So additional work is still required.
Comments and reviews welcome.
Reference #1246
Reference #1242
Reference NationalSecurityAgency/ghidra#7141
Reference py5coding/py5generator#563
This PR only drops the agent requirement and cleans up the class loader issues for non-ascii paths. PR #1246 deals with the broken JVMs on Windows.