Add 'setClassLoader' with argument 'forceClassLoader' #570

matthinc · 2019-10-13T19:25:49Z

The external ClassLoader can be used as a security measure to prevent the execution of unwanted code.
The problem is, that bsh in its current state uses the internal ClassLoader if the external ClassLoader fails. I added an optional forceClassLoader argument to setClassLoader, which prevents bsh from proceeding if the external ClassLoader fails.

* implement the JDK7, aka project coin, number literal additions * add binary number literals ex. 0b010101 * correctly parsing and processing underscores in numeric literals

WeakHashMap would either loose or not correctly set the namespace in multi thread conditions. Instead lets use ConcurrentHashMap and manually remove the entries on retrieval.

It was not implemented anymore so remove it.

Removed in favour of JSR223 support. BSF engine moved to external module bsh-bsf-engine for anyone that might still require it.

The field attribute is not serializable so when writing object we drop the field and retain the declared class, when needed again we fetch the field from the class again.

* test is successful if we remove the assert(false); * removed additional junit test not required * remove as known issue

* rename to BshScriptTestCase and inner class TestBshScript to Script * add Script constructor for String filename in test script folder * simplify setting interpreter "bsh.cwd" variable * properly allow Fail.bsh to fail and assert expected error * add ability to skip tests based on assumptions

We'll assume we haveAccessibility before attempting to test illegal access, this will skip these tests if accessibility=false (default)

In some cases static imports were failing now we look for the class first before leaving it to Name to try and resolve.

Even with setAccessible package private constructors and methods were failing, we now make constructors and methods without modifiers public and super delegates are public by default as they will have any additional modifiers applied when calling the super method.

JDK8+ does not have rt.jar anymore so we just add an empty boot class path. load command does not need to setAccessible anymore. compile and run tests with accessibility=false (default) now succeeds with --illegal-access=deny with Java 9 or Java 10

It is not clear what was originally intended with this script it fails for lack of a default constructor on the anonymous class. The current state of anonymous class capability is uncertain and since this is called classinner and not classanonymous I added something more appropriate to test for. Grouped some more scripts with the InnerClassTest test case for reference.

This test was failing on the if statement Capabilities.classExists("bsh.ExternalNameSpace") for some reason. Added a static import and changed the if block to an assume and all the asserts passed. Refactored the test to use junit asserts seemed more appropriate.

* removed some commented lines * ph-javacc-maven-plugin version 4.x uses the new javacc fork ParserGeneratorCC see https://github.com/phax/ParserGeneratorCC * TokenMgrError was made redundant by ParserGeneratorCC so we remove it as well * added bsh.debugClasses as a system property for specifying the location to save generated class files.

* for null type we give preference to String, then most specific non primitive which is not an array, then arrays * less specific is assignable by most specific unless signature is a match * improved varargs matching no argument or null type as vararg * known issue RefectTest.findMostSpecificSignature elaborated and resolved

* added some reusability improvements to testUtil * renamed and redistributed issue_XX tests to more intuitive names.

Collected from generically named unit tests.

Only contains the switch string statement now.

* added new grammar for try TryWithResources and AutoCloseable * implemented BSHAutoCloseable as a subclass of BSHTypedVariableDeclaration to reuse the variable evaluation. * BSHTryWithResources acts as the collection of resources and interface for evaluating and closing the resources. * added a lookup for 1st Block node to BSHTryStatement to avoid the use of magic numbers * on eval start if the 1st Node is TryWithResources we keep a reference and call eval which will create the variables in the namespace in callstack * in the finally block of the try block evaluation, if we have a reference to BSHTryWithResources we auto close, which ensures that all resources are closed. * suppressed on close exceptions will be collected in the target exception from the try block

* Infer a collection of parameter names and set a collection of parameter modifiers for the BshMethod wrapped java.reflect.Method delegation.

Java 9/10 handles the close exceptions differently to JDK 8, with the latter the exceptions thrown on close are the same instance as the target exception with suppressed exception already collected. With the newer versions you have to add the suppressions manually.

* add grammar to parse multi catch * implement evaluation of catch block with a list of types * improve check assignability to avoid the previous try fail catch continue manoeuvres * enable ability to throw throwable and subtypes - previously limited to exception types only * added some simple multi catch tests

We are loosing state because some nodes don't have modifiers.

* remove redundant use of token variables * remove redundant comments * some styling ease of readability

default behaviour ,regardless of accessibility, if no access modifiers (or package private) bsh default is public visibility

* validate abstract methods are implemented or class declared abstract * bsh classes are virtual abstract only or iow abstract classes can be instantiated * apply inheritance rules overridden methods may not reduce visibility

Added additional eval only constructors for Interpreter which uses the system standard output and standard error but defers input to be supplied by eval.

Refactored eval method to extract reusable method for terminating statement strings.

Simplified the implementation of the PreparsedScript class, simple interpreter instance apply class loader to interpreter, eval the script to retrieve the callable object and reference the prepared instance. On invoke create the scoped name space and local interpreter, set the context variables invoke the prepared script and unwrap the results. Added compound name resolution in context variable names. Standardised the source file info and do not reveal the wrapped method name. Added terminated script conversion to ensure statement is terminated fixes beanshell#529 Fixed unit tests and enabled multi threaded test, added additional tests to illustrate compound name resolution. Added serial garbage collector java vm argument to properly apply large eden algorithms which parallel gc will stall on. Added an empty map and a map of key value pair list helper to TestUtil for supplying tests with data maps.

Changes as per enhancements made to StringUtil for data type values to string. Fixes beanshell#527

Performance: O(1) Listener add/delete

Fix for beanshell#566 cannot concat assign null string.

It was not immediately clear what the purpose of checkNullValues was so refactored the implementation. Optimised if statements to release as soon as possible by finding most likely to fail first. Found and fixed bug with null comparison with wrapper types not working like Java. Now allows for comparing 3 states of Boolean types. Reduce operators required stack memory footprint.

codecov · 2019-10-13T19:41:18Z

Codecov Report

Merging #570 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #570      +/-   ##
============================================
- Coverage     70.44%   70.40%   -0.04%     
+ Complexity     2695     2694       -1     
============================================
  Files           106      106              
  Lines          8945     8948       +3     
  Branches       1765     1765              
============================================
- Hits           6301     6300       -1     
- Misses         2215     2217       +2     
- Partials        429      431       +2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/bsh/BshClassManager.java	`64.67% <ø> (-1.50%)`	`22.00 <0.00> (-1.00)`
src/main/java/bsh/classpath/ClassManagerImpl.java	`64.11% <100.00%> (-0.56%)`	`38.00 <0.00> (-1.00)`
src/main/java/bsh/commands/dir.java	`58.49% <0.00%> (-1.89%)`	`4.00% <0.00%> (ø%)`
src/main/java/bsh/Name.java	`85.58% <0.00%> (+0.58%)`	`117.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6014e15...be47004. Read the comment docs.

nickl-

Awesome thanx for the PR.

Made some suggestions please see review comments.

nickl- · 2019-11-13T10:25:18Z

src/main/java/bsh/classpath/ClassManagerImpl.java

+                // If forceExternalClassLoader is true, there is no reason to return a class here
+                if (forceExternalClassLoader) {
+                    return null;
+                }
            }
        }



We can do the check once on this line and return c;

Also I'd prefer one line statements without braces please.

Also we need to hook the cache storage here before returning.

// Cache result (or null for not found) cacheClassInfo( name, c );

I added the caching. The brace suggestion is obsolete, because I need more lines now.

video call for the core 2

nickl- · 2019-11-13T10:27:21Z

src/main/java/bsh/BshClassManager.java

@@ -546,8 +552,20 @@ protected void clearCaches() {
        However BeanShell is not currently able to reload
        classes supplied through the external classloader.
    */
-    public void setClassLoader( ClassLoader externalCL ) {
+    public void setClassLoader(ClassLoader externalCL) {


Try to avoid white space changes, it makes it easier to review.

We will apply a uniform code style at a later stage.

nickl- · 2019-11-13T10:47:10Z

src/test/java/bsh/classpath/ExternalClassLoaderTest.java

+    public void testExternalClassManagerOptional() throws EvalError {
+        final Interpreter interpreter = new Interpreter();
+        interpreter.setClassLoader(testLoader);
+        interpreter.eval("String tst = \"Hello, World!\";");


No assertion in test.

Sure you are testing that it doesn't fail but that is not clear from inspecting the test case.

Assert that Class.getClassLoader() doesn't equal the same instance as proof.

Perhaps also test for Thread.currentThread().getContextClassLoader()

I rewrote all the tests. They work with try/catch now. You're right - they were hard to understand.

src/test/java/bsh/classpath/ExternalClassLoaderTest.java

nickl- · 2019-11-13T10:58:19Z

src/test/java/bsh/classpath/ExternalClassLoaderTest.java

+        @Override
+        public Class<?> loadClass(final String name) throws ClassNotFoundException {
+            if (name.equals("java.lang.String")) {
+                throw new ClassNotFoundException();


After some more thought I don't like this.

Throwing exceptions is expensive and a much more thorough test would be to return null.

Good point, but I have also seen people, who used a ClassNotFoundException.
I now have both - null and the exception as a test-case.

nickl- · 2019-11-13T11:02:34Z

src/test/java/bsh/classpath/ExternalClassLoaderTest.java

+    public void testExternalClassManagerForced() throws EvalError {
+        final Interpreter interpreter = new Interpreter();
+        interpreter.setClassLoader(testLoader, true);
+        interpreter.eval("String tst = \"Hello, World!\";");


Eval can return tst for assert.

It came to me, that a String is not a good test case, because it's handled differently than other classes by java. I now use 'System' and 'Math' - they should be much better test-cases.

src/main/java/bsh/classpath/ClassManagerImpl.java

.travis.yml

nickl- · 2019-11-13T12:31:52Z

I've given this some more thought, why is this so complicated? Now we're complicating it even more.

Can't we choose a class loader on initialize, first from Thread.currentThread().getContextClassLoader(); then from this.getClass().getClassLoader() (which is what forName() will use anyway) if it fails. Otherwise use the one specified by setClassLoader(). If you want to load a class from source file you can explicitly source the file with interpreter before use.

Doesn't that make more sense?

matthinc · 2019-11-19T17:33:49Z

I've given this some more thought, why is this so complicated? Now we're complicating it even more.

Can't we choose a class loader on initialize, first from Thread.currentThread().getContextClassLoader(); then from this.getClass().getClassLoader() (which is what forName() will use anyway) if it fails. Otherwise use the one specified by setClassLoader(). If you want to load a class from source file you can explicitly source the file with interpreter before use.

Doesn't that make more sense?

Hi @nickl-,
I agree with you that the current logic is pretty complicated. The way how the setClassLoader works now is confusing and could be even dangerous in some situations.
My goal was to force the use of a ClassLoader without breaking any existing code which depends on bsh.
Alternatively, I could change the behavior, that setClassLoader aways forces the use of the ClassLoader and clean up the existing logic. But that would possibly break some projects.

What would you suggest?

https://central.sonatype.org/pages/apache-maven.html

nickl- · 2020-01-18T15:33:39Z

Alternatively, I could change the behavior, that setClassLoader aways forces the use of the ClassLoader and clean up the existing logic. But that would possibly break some projects.

What would you suggest?

I think we should rework the process by simplifying as suggested. Give it a try and if it breaks anything, which I doubt it will, then lets take it from there.

Include more unit tests if code coverage is not sufficient.

Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 8.10 to 8.29. - [Release notes](https://github.com/checkstyle/checkstyle/releases) - [Commits](checkstyle/checkstyle@checkstyle-8.10...checkstyle-8.29) Signed-off-by: dependabot[bot] <support@github.com>

…bsf-engine/com.puppycrawl.tools-checkstyle-8.29 Bump checkstyle from 8.10 to 8.29 in /bsh-bsf-engine

matthinc · 2020-02-12T08:43:53Z

Thank you for all the feedback!
I applied your suggestions to my PR

nickl- · 2023-01-07T18:22:10Z

@matthinc
I tried to rebase this on a separate branch but it looks to be too outdated. Please rework if you don't want to loose the commits or I will have to manually apply the changes to master.

nickl- added 30 commits March 19, 2019 20:34

Implement JDK7 number literal additions.

6af9711

* implement the JDK7, aka project coin, number literal additions * add binary number literals ex. 0b010101 * correctly parsing and processing underscores in numeric literals

Fix concurrency issues with pullBshStatic lazy initialisation.

08ee11e

WeakHashMap would either loose or not correctly set the namespace in multi thread conditions. Instead lets use ConcurrentHashMap and manually remove the entries on retrieval.

Removed LOCALSCOPING

d7dd857

It was not implemented anymore so remove it.

Remove BSF2.x support.

600fcde

Removed in favour of JSR223 support. BSF engine moved to external module bsh-bsf-engine for anyone that might still require it.

Convert TestBshScriptEngine to junit test.

41af76b

Amend Implement variable declared array.

cd5b54d

Fix LHS Serializability.

cff58ca

The field attribute is not serializable so when writing object we drop the field and retain the declared class, when needed again we fetch the field from the class again.

class3.bsh test is working

df0db64

* test is successful if we remove the assert(false); * removed additional junit test not required * remove as known issue

Skip accessibility tests if haveAccessibility false.

6c6e704

We'll assume we haveAccessibility before attempting to test illegal access, this will skip these tests if accessibility=false (default)

Fix import static

9627f3a

In some cases static imports were failing now we look for the class first before leaving it to Name to try and resolve.

Refactor TestUtil and rename issue_XX tests.

7b02565

* added some reusability improvements to testUtil * renamed and redistributed issue_XX tests to more intuitive names.

Extract NumberLiteralTest

a5c48ca

Collected from generically named unit tests.

Move try with resources tests to TryStatementTest

98c4c97

Rename Project_Coin_Test to StatementsTest

bc172a6

Only contains the switch string statement now.

Checkstyle and cosmetics

f6b5d18

Infer wrapped method missing items

1243402

* Infer a collection of parameter names and set a collection of parameter modifiers for the BshMethod wrapped java.reflect.Method delegation.

Ensure that members have modifiers on node.

2e1904a

We are loosing state because some nodes don't have modifiers.

Grammar clean up.

0b7a318

* remove redundant use of token variables * remove redundant comments * some styling ease of readability

Improved no access modifier is public implementation.

892d635

default behaviour ,regardless of accessibility, if no access modifiers (or package private) bsh default is public visibility

nickl- and others added 10 commits June 25, 2019 03:22

Additional Interpreter constructors.

35911c3

Added additional eval only constructors for Interpreter which uses the system standard output and standard error but defers input to be supplied by eval.

Extract method terminatedScript.

6a3e6d0

Refactored eval method to extract reusable method for terminating statement strings.

Updates to print command.

542bd0e

Changes as per enhancements made to StringUtil for data type values to string. Fixes beanshell#527

Performance: O(1) Listener add/delete

0ae2c1c

Merge pull request beanshell#562 from morris821028/master

800e6c0

Performance: O(1) Listener add/delete

Fix plus assign null string.

25427cb

Fix for beanshell#566 cannot concat assign null string.

Add 'setClassLoader' with argument 'forceClassLoader'

e16ea4a

Fix .travis.yml

28d09fe

nickl- requested changes Nov 13, 2019

View reviewed changes

stain added 7 commits December 16, 2019 15:22

[maven-release-plugin] prepare release 3.0-alpha-1

37688c7

[maven-release-plugin] prepare for next development iteration

a9da27a

Deploy directly to OSS Sonatype instead of BinTray

fabaa5d

https://central.sonatype.org/pages/apache-maven.html

Avoid jdk_switcher

7fb24c2

Use Travis dist xenial to get modern Java

ef21e91

oracle JDK 8 not available, but we can be brave with 14!

318d6d6

test against JDK 13, 14 is not yet released

53be487

nickl- mentioned this pull request Jan 18, 2020

Class Loading Performance Issue #579

Open

dependabot bot and others added 4 commits January 31, 2020 18:05

Merge pull request beanshell#582 from beanshell/dependabot/maven/bsh-…

6014e15

…bsf-engine/com.puppycrawl.tools-checkstyle-8.29 Bump checkstyle from 8.10 to 8.29 in /bsh-bsf-engine

Force the use of an external ClassLoader - Apply PR suggestions

27d526a

Merge branch 'master' into master

be47004

patniemeyer force-pushed the master branch from 10e0089 to b35aac4 Compare December 6, 2023 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'setClassLoader' with argument 'forceClassLoader' #570

Add 'setClassLoader' with argument 'forceClassLoader' #570

matthinc commented Oct 13, 2019

codecov bot commented Oct 13, 2019 •

edited

nickl- left a comment

nickl- Nov 13, 2019

nickl- Nov 13, 2019

matthinc Feb 12, 2020

rahul28headchannel Jan 27, 2022

rahul28headchannel Jan 27, 2022

nickl- Nov 13, 2019

nickl- Nov 13, 2019

nickl- Nov 13, 2019

nickl- Nov 13, 2019

matthinc Feb 12, 2020

nickl- Nov 13, 2019

matthinc Feb 12, 2020

nickl- Nov 13, 2019

matthinc Feb 12, 2020

nickl- commented Nov 13, 2019

matthinc commented Nov 19, 2019

nickl- commented Jan 18, 2020

matthinc commented Feb 12, 2020

nickl- commented Jan 7, 2023

Add 'setClassLoader' with argument 'forceClassLoader' #570

Are you sure you want to change the base?

Add 'setClassLoader' with argument 'forceClassLoader' #570

Conversation

matthinc commented Oct 13, 2019

codecov bot commented Oct 13, 2019 • edited

Codecov Report

nickl- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickl- commented Nov 13, 2019

matthinc commented Nov 19, 2019

nickl- commented Jan 18, 2020

matthinc commented Feb 12, 2020

nickl- commented Jan 7, 2023

codecov bot commented Oct 13, 2019 •

edited