Lucene - Core
  1. Lucene - Core
  2. LUCENE-6920

Simplify callable function checks in Expression module

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: modules/expressions
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The expressions module allows to specify custom functions. It does some checks to ensure that the compiled Expression works correctly and does not produce linkage errors. It also checks parameters and return type to be doubles.

      There are two problems with the current approach:

      • the check gets classloaders of the method's declaring class. This fails if a security manager forbids access to bootstrap classes (e.g., java.lang.Math)
      • the code only checks if method or declaring class are public, but not if it is really reachable. This may not be the case in Java 9 (different module without exports,...)

      This issue will use MethodHandles to do the accessibility checks (it uses MethodHandles.publicLookup() to resolve the given reflected method). If that fails, our compiled code cannot acess it. If module system prevents access, this is also checked.

      To fix the issue with classloaders, it uses a trick: It calls Class.forName() with the classloader we use to compile our expression. If that does not return the same class as the declared method, it also fails compilation. This prevents NoClassDefFoundException on executing the expression.

      All tests pass.

      1. LUCENE-6920.patch
        6 kB
        Uwe Schindler
      2. LUCENE-6920.patch
        5 kB
        Uwe Schindler
      3. LUCENE-6920.patch
        5 kB
        Uwe Schindler

        Activity

        Hide
        Uwe Schindler added a comment -

        Patch. All tests pass.

        Show
        Uwe Schindler added a comment - Patch. All tests pass.
        Hide
        Robert Muir added a comment -

        +1, looks good.

        We may also try removing the hack for it in tests.policy...

          // expressions TestCustomFunctions (only on older java8?)
          permission java.lang.RuntimePermission "getClassLoader";
        

        The reason for the comment there: the last time i looked at this, there was some differences with java versions that confused me. So if we are worried about that, we could also just fix it for trunk only and avoid any java 7 problems.

        Show
        Robert Muir added a comment - +1, looks good. We may also try removing the hack for it in tests.policy... // expressions TestCustomFunctions (only on older java8?) permission java.lang.RuntimePermission "getClassLoader" ; The reason for the comment there: the last time i looked at this, there was some differences with java versions that confused me. So if we are worried about that, we could also just fix it for trunk only and avoid any java 7 problems.
        Hide
        Uwe Schindler added a comment -

        Thanks for the hint. There is another getClassLoader and getClassLoader.getParent() currently in the SPI classloader. I will check this out tomorrow and open a separate issue to protect this with doPrivileged(). But this one is uncritical, as it only tries to get classloader for cases where the context class loaders differs....

        I may also add a test using LTC#runWithRestrictedPermissions() to ensure that this really needs no privileges.

        Show
        Uwe Schindler added a comment - Thanks for the hint. There is another getClassLoader and getClassLoader.getParent() currently in the SPI classloader. I will check this out tomorrow and open a separate issue to protect this with doPrivileged(). But this one is uncritical, as it only tries to get classloader for cases where the context class loaders differs.... I may also add a test using LTC#runWithRestrictedPermissions() to ensure that this really needs no privileges.
        Hide
        Uwe Schindler added a comment - - edited

        New patch with permission removed. Solr never had this permission.
        When backporting I will for sure also check Java 7, but I don't think there are problems.

        Show
        Uwe Schindler added a comment - - edited New patch with permission removed. Solr never had this permission. When backporting I will for sure also check Java 7, but I don't think there are problems.
        Hide
        Uwe Schindler added a comment -

        Small cleanup: I put the classloader check into a separate method, only called for external user functions, not for our default ones.
        Will commit this soon.

        Show
        Uwe Schindler added a comment - Small cleanup: I put the classloader check into a separate method, only called for external user functions, not for our default ones. Will commit this soon.
        Hide
        ASF subversion and git services added a comment -

        Commit 1718069 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1718069 ]

        LUCENE-6920: Improve custom function checks in expressions module to use MethodHandles and work without extra security privileges

        Show
        ASF subversion and git services added a comment - Commit 1718069 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1718069 ] LUCENE-6920 : Improve custom function checks in expressions module to use MethodHandles and work without extra security privileges
        Hide
        Uwe Schindler added a comment -

        Java 7 also works fine.

        Show
        Uwe Schindler added a comment - Java 7 also works fine.
        Hide
        ASF subversion and git services added a comment -

        Commit 1718070 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1718070 ]

        Merged revision(s) 1718069 from lucene/dev/trunk:
        LUCENE-6920: Improve custom function checks in expressions module to use MethodHandles and work without extra security privileges

        Show
        ASF subversion and git services added a comment - Commit 1718070 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1718070 ] Merged revision(s) 1718069 from lucene/dev/trunk: LUCENE-6920 : Improve custom function checks in expressions module to use MethodHandles and work without extra security privileges
        Hide
        Uwe Schindler added a comment -

        Thanks Robert for review & suggestions.

        Show
        Uwe Schindler added a comment - Thanks Robert for review & suggestions.
        Hide
        Uwe Schindler added a comment -

        I opened LUCENE-6921 for the other problemtic check in SPI.

        Show
        Uwe Schindler added a comment - I opened LUCENE-6921 for the other problemtic check in SPI.

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development