Let SQL generator match implementor non-case-folded form to provides string
#518
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JEP 472 issues warnings (one day to be errors) for uses of System.loadLibrary or when native methods are first bound according to the default binding process. PL/Java's use of JNI escapes without warnings because the JVM is launched from C and uses RegisterNatives to bind the native methods explicitly, so it does not involve loadLibrary or any use of the default native method matching. On the other hand, the pgjdbc-ng library furnishes netty, which has a use of loadLibrary, and produces warnings from the client side in the CI script. As Node.java now supports both pgjdbc and pgjdbc-ng, switch to pgjdbc in the CI scripts to avoid a nuisance warning.
8a6b1b4 noted that the nearly-identical blobs of jshell in three different CI service configs would be ripe for factoring out into one common version, but left that chore for another day. This may as well be the day.
Since JEP 471 in Java 23, --sun-misc-unsafe-memory-access=deny can be used. Since JEP 472 in Java 24, --illegal-native-access=deny can be used. Both would be best practice, especially in Java 24+ where no policy is available to control access to Unsafe or loadLibrary. Refactor the test script slightly so the Java major version and PostgreSQL major version aren't repeatedly obtained.
None of this is related at all to JEP 472, but some is needed to catch up with changes on the GitHub runners as the CI would otherwise fail. The macos-12 runner is gone, and it was the last macos one that supplied PostgreSQL preinstalled. So rename the workflow from ci-runnerpg to ci-lazypg ... it still uses the preinstalled PG on runners that have one, but now resorts to brew to install it on macos. The macos-13 runner is still Intel-based (and needs PG installed). The macos-14 runner is ARM64-based (and needs PG installed). The runners have some Java versions preinstalled, so check for those and only fetch a JDK if the matrix.java version isn't one of the preinstalled ones. Preinstalled versions can be found by their JAVA_HOME_$VERSION_$ARCH environment variables, except that when $ARCH is ARM64 the environment variable has arm in lowercase. So much for consistency. There are newer versions of the setup-java and checkout actions. The old ones elicited warnings about a deprecated Node.js version. The old setup-java, when running on macos ARM64 (or arm64), would happily install an x86_64 JDK. Which would seem to work fine, apparently running in a compatibility mode, apparently inherited by the C compiler and linker spawned from Maven. The linker would then report baffling errors about not finding symbols in PostgreSQL for x86_64! This is the exact sort of mystery seen and not yet solved in issues #411 and #428. So, running Maven on a JDK that's for the wrong architecture is now a known way to cause such a problem, and worth checking in such a case. Added some ::group:: / ::endgroup:: to make the log output more easily perused.
Also, seems there's no longer a 9 (or 10) JDK version either preinstalled or fetchable for a GitHub runner.
Miscellaneous keeping up with GitHub Actions changes so the JEP472 changes can successfully run.
Completes pull request #511.
JEP 486 finally disallows any means of installing a security manager, as of Java 24, by making 'disallow' the only allowable property value for java.security.manager (other than unset, which is the default and means the same thing, but is distinguishable). Therefore, PL/Java can require the admin to set this explicitly to 'disallow' to indicate "I want to use Java 24+ and, yes, I understand there will be no policy enforcement of any kind." Revise the error / warning / notice messages about the change, now that it's no longer unclear just what form the disabling of functionality will take. Avoid exposing sensitive values such as datadir, libdir, codesource as properties in the no-enforcement mode, as nothing will protect those properties anymore from being read or changed.
In the no-enforcement mode selected with java.security.manager=disallow, enforce (in the validator) that function creation even in a PL flagged 'trusted' is reserved to superusers. This check is made unconditionally, right after CheckFunctionValidatorAccess but before the short-circuit return made if that returns false (a reserved possible future case), and before consulting check_function_bodies. Naturally, any functions already defined in 'trusted' PLs, prior to a PL/Java and/or Java update newly configured for no enforcement, remain defined. Selectively enabling those to be executable in no-enforcement mode will be the subject of the next patch.
The value is a comma-separated list of PL names (such as java, javau, possibly others created with sqlj.alias_java_language). Only functions defined in the PLs named here will be allowed to execute when the java.security.manager=disallow property setting is in effect. This will also be checked in the validator, thus preventing creation of a new function if the PL is not named here. This is checked in the validator only when check_function_bodies is on, so it is better viewed as a reminder than as meaningful security. The check at function invocation is the important one.
The JVM has long has hooks for both, just as sparsely documented as the vfprintf hook, which PL/Java has already long used. This is a good time to start hooking abort and exit also, in part because of the seemingly ever-growing set of JVM startup issues that get reported with an abort instead of a civilized error return from JNI_CreateJavaVM, and in part because if there's to be no policy enforcement around System.exit(), one likes at least to know what has happened if it gets called. (Regrettably, the exit hook is not given the option to just say "no".)
This one is a simple boolean. The Java readSQL/writeSQL methods of a MappedUDT get called by PL/Java directly, not through declared SQL functions with an associated language name, so they slip through the cracks of pljava.allow_unenforced based on language name. No MappedUDT readSQL/writeSQL methods will be executed if this setting is off. They'll be executed normally (to the extent "without enforcement" can be called "normally") when it is on.
Maybe also phrase the Backend.c JEP 411 warning a little bit less apocalyptically. Having suggested --limit-modules=org.postgresql.pljava.internal and claimed that it doesn't break the tests, add it to the CI script to make sure it stays that way.
There are so many things resting on assumptions that various properties aren't just freely modifiable by any code anywhere. Add an unmodifiable FrozenProperties final subclass of Properties, and populate it with a defensive copy just after InstallHelper has set the PL/Java-specific properties. Make this frozen copy available to user code through the Session API. The above is not enough by itself, because PL/Java internally uses some properties in code that executes before that point in PL/Java startup. So those places have to be fixed to do their own defensive caching. ELogFormatter is easy because writing a newline can be done with PrintWriter.println(), and that uses a copy of line.separator already cached early in the JVM startup. The determination of the byte ordering for SQLInputFromChunk and SQLOutputToChunk is more work. Just move all that logic out of InstallHelper and into a new class that just serves as a cache for that, and arrange for the new class to be initialized at the right point in PL/Java startup.
It is distinguishable whether a property was defaulted rather than explicitly set (getProperty(n) finds it but containsKey(n) does not), so the same should be true of the FrozenProperties.
The hardening advice in unenforced.md refers to the number of modules resolved into the boot layer, but didn't offer an easy way to check.
The table from the pull request #512 comment would be useful to have in the actual docs.
While PL/Java's docs are meant to build (mvn site site:stage) without failure on any Java version supported for building PL/Java, there have been changes over the Javadoc versions to things like the output directory structure and the spelling of anchor names for use as URL fragment IDs. Therefore, it's necessary to pick a version of Javadoc to be used when generating the docs if such things as links from the Markdown documents into the generated javadocs are to work right. Links from the generated javadocs to Oracle's published JDK javadocs are made to the Java 12 pages as a practical compromise: PL/Java 1.6.x is built for compatibility back to Java 9, but Oracle's published pages for Java 12 are the earliest to have the organization expected by a modern Javadoc. The PL/Java examples (non-modular code) have Javadoc links back to the PL/Java API (modular code), which Javadoc before 15 doesn't know how to do. Later Javadoc versions have some further aesthetic improvements. For now, assume that Javadoc 21 (the recent LTS version) will be used for generating the site javadocs, and adjust the URLs that point into the javadocs accordingly.
A slightly Orwellian feeling is probably no important reason not to also fix links in releasenotes-pre1_6 so they aren't broken when the docs are built with a present-day javadoc. There will still be unavoidable breakage given that anchor names for functions are generated by recent Javadoc versions to include parentheses, which are sanitized out of links by Velocity's Markdown processor.
1032e68 wrapped some JavaScript code in function execute() { ... } which was exdented, to avoid reindenting everything inside, probably with the intention to have a later reformat-only commit making the indentation consistent again. This happened both in pljava-packaging/pom.xml and in pljava-so/pom.xml. In the first, there was no code outside of what became the function body, so the result was just to have the content of the CDATA section start at the left margin. It looks fine. So reformat pljava-so/pom.xml the same way, moving the lines outside the function back to the margin, rather than further indenting the function and its body. This reformatting is not totally mechanical: some line breaks have been changed to stay within 80 columns, and one java.util.List.of shortened to of, having apparently been overlooked in 9111286.
The Windows runner comes with an MSVC-built PostgreSQL preinstalled (probably the Chocolatey one, which relies on the EnterpriseDB installer). The fussiest part of configuration is finding the Visual Studio .bat file that sets all the environment variables needed for the build to succeed, but there is an action ilammy/msvc-dev-cmd that takes care of finding and running that, capturing its changes to the environment, and replicating those in the environment of the subsequent steps. For some reason java.home on Windows has the JVM shared object under bin\server and not lib/server as on Linux.
MSYS2 is preinstalled on the runner but apparently not with gcc. Have pacman install that also, and prepend the c:\msys64\mingw64\bin directory to the PATH. Using msys2bash, it has to be prepended in the form /c/msys64/mingw64/bin, which gets transformed correctly into the Windows environment variable. The gcc that is installed on the runner is in /c/mingw64/bin rather than /c/msys64/mingw64/bin where pacman puts it. A lot of cycles and bandwidth would be saved if that one could be used. PL/Java can be successfully built that way, but on loading it into the backend, "server process (PID 2116) was terminated by exception 0xC0000005". So it seems that building to load into a pacman PostgreSQL backend must be done using the pacman gcc.
Use exclude: to test fewer Java versions on the Windows runners, which seem to be extra slow.
A couple documentation changes and a diagnostic message improved, overlooked in PR #512 (d28e9fa). Pushing these without another PR. The Java-version condition determining the exact message shown when beginEnforcing fails is now harmonized with what install/smproperty.md says about use of the java.security.manager property by Java version. The property must be supplied starting with Java 18, so the useful hint should be given for failures on 18 through 23, rather than calling the failure unexpected.
The DDRProcessor creates an implied dependency of an 'implementor' name on the SQLAction carrying a matching 'provides' string. An implementor name, per ISO SQL/JRT, is an SQL identifier, case-insensitive unless quoted. But a PL/Java 'provides' string is just a string, with only case-sensitive exact matching. The DDRProcessor was using a surprising and undocumented rule, matching only if the provides string was identical to the implementor name's lower-case-folded form. The results could be especially puzzling because the 'weak' nature of the implementor/provides dependency meant there was no error or warning of the failue to match. The dependency simply wasn't created, leading possibly to an incorrectly-ordered deployment descriptor. Rather than some more general and complicated solution, simply change the rule so the provider string is matched to the non-folded implementor name as it was spelled in the source annotation. Thus as long as the programmer does the expected thing and spells them the same in the source, the right thing happens. Addresses #515.
This had never been well documented. The javadoc of the ConditionalDDR example code was about the best there was.
Contributor
Author
|
Closed because opened against the wrong base branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
DDRProcessorcreates an implied dependency of animplementorname on theSQLActioncarrying a matchingprovidesstring. An implementor name, per ISO SQL/JRT, is an SQL identifier, case-insensitive unless quoted. But a PL/Javaprovidesstring is just a string, with only case-sensitive exact matching. TheDDRProcessorwas using a surprising and undocumented rule, matching only if theprovidesstring was identical to theimplementorname's lower-case-folded form.The results could be especially puzzling because the 'weak' nature of the
implementor/providesdependency meant there was no error or warning of the failure to match. The dependency simply wasn't created, leading possibly to an incorrectly-ordered deployment descriptor.Rather than some more general and complicated solution, simply change the rule so the
providesstring is matched to the non-foldedimplementorname as it was spelled in the source annotation. Thus as long as the programmer does the expected thing and spells them the same in the source, the right thing happens.Addresses issue #515.