Lucene - Core
  1. Lucene - Core
  2. LUCENE-4202

allow check-forbidden-apis to look for fields too

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-BETA
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Currently this supports classes and methods, but there are some deprecated fields in the java API, it would be nice to check for those, too.

      1. LUCENE-4202.patch
        15 kB
        Uwe Schindler
      2. LUCENE-4202.patch
        7 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          I just wanted to add my recent blog post on this issue as further documentation: http://blog.thetaphi.de/2012/07/default-locales-default-charsets-and.html

          Show
          Uwe Schindler added a comment - I just wanted to add my recent blog post on this issue as further documentation: http://blog.thetaphi.de/2012/07/default-locales-default-charsets-and.html
          Hide
          Hoss Man added a comment -

          hoss20120711-manual-post-40alpha-change

          Show
          Hoss Man added a comment - hoss20120711-manual-post-40alpha-change
          Hide
          Robert Muir added a comment -

          Fixed (hopefully) the problem with ANT's .lib/ folder. The task now only uses system classloader and its own one, while the own one is used in preference (which violates spec); but is much better for our checks (which do not rely on JVM class loading semantics).

          I think this is ready to commit, Robert can you check with Apache RAT installed?

          I checked: it works!

          Show
          Robert Muir added a comment - Fixed (hopefully) the problem with ANT's .lib/ folder. The task now only uses system classloader and its own one, while the own one is used in preference (which violates spec); but is much better for our checks (which do not rely on JVM class loading semantics). I think this is ready to commit, Robert can you check with Apache RAT installed? I checked: it works!
          Hide
          Uwe Schindler added a comment -

          Committed trunk revision: 1359590
          Backported 3.x revision: 1359592

          Show
          Uwe Schindler added a comment - Committed trunk revision: 1359590 Backported 3.x revision: 1359592
          Hide
          Uwe Schindler added a comment -

          For the inheritance problem I opened LUCENE-4206 (I think I have a suitable solution). But this issue must be committed first, which I will do now.

          Show
          Uwe Schindler added a comment - For the inheritance problem I opened LUCENE-4206 (I think I have a suitable solution). But this issue must be committed first, which I will do now.
          Hide
          Uwe Schindler added a comment -

          Separately we could open an issue to deal with this virtual problem? It just means our checker isn't as thorough as it is, but none of our checkers/tests are perfect.

          Yes it is not as thorough for things like Throwable.printStackTrace(), but for the JVM's deprecated list, we should still catch almost all, as the deprecated list in the JVM is complete (it also should list subclasses - if method is overridded).

          For the original checks we did at beginning (encoding, locale,... problems), this was also thorough enough, as we listed all calls that may be used. If a custom subclass in Lucene code would subclass this system class and modify a "forbidden method", the "super" call would trigger the violation report.

          I think this is ready to commit, Robert can you check with Apache RAT installed?

          Show
          Uwe Schindler added a comment - Separately we could open an issue to deal with this virtual problem? It just means our checker isn't as thorough as it is, but none of our checkers/tests are perfect. Yes it is not as thorough for things like Throwable.printStackTrace(), but for the JVM's deprecated list, we should still catch almost all, as the deprecated list in the JVM is complete (it also should list subclasses - if method is overridded). For the original checks we did at beginning (encoding, locale,... problems), this was also thorough enough, as we listed all calls that may be used. If a custom subclass in Lucene code would subclass this system class and modify a "forbidden method", the "super" call would trigger the violation report. I think this is ready to commit, Robert can you check with Apache RAT installed?
          Hide
          Uwe Schindler added a comment -

          New patch with:

          • Spaghetti code removal in signature parser
          • Fixed (hopefully) the problem with ANT's .lib/ folder. The task now only uses system classloader and its own one, while the own one is used in preference (which violates spec); but is much better for our checks (which do not rely on JVM class loading semantics).
          • Better error reporting
          • Print log message for each loaded signature file
          • Cleaned up Solr classpath to only inspect lib/ folder when loading signatures.
          Show
          Uwe Schindler added a comment - New patch with: Spaghetti code removal in signature parser Fixed (hopefully) the problem with ANT's .lib/ folder. The task now only uses system classloader and its own one, while the own one is used in preference (which violates spec); but is much better for our checks (which do not rely on JVM class loading semantics). Better error reporting Print log message for each loaded signature file Cleaned up Solr classpath to only inspect lib/ folder when loading signatures.
          Hide
          Uwe Schindler added a comment - - edited

          Robert, I have no solution until now. I fogot to mention yesterday: You can add your fields and then commit this you like.

          Show
          Uwe Schindler added a comment - - edited Robert, I have no solution until now. I fogot to mention yesterday: You can add your fields and then commit this you like.
          Hide
          Robert Muir added a comment -

          Can't we separate these issues though?

          I think its ok to add the Fields check here, and populate the deprecated fields (and maybe consider a separate ant instantiation that checks non-test fileset for System.out & co).

          Separately we could open an issue to deal with this virtual problem? It just means our checker isn't as thorough as it is, but none of our checkers/tests are perfect.

          This all assumes Uwe doesn't wake up with a great solution

          Show
          Robert Muir added a comment - Can't we separate these issues though? I think its ok to add the Fields check here, and populate the deprecated fields (and maybe consider a separate ant instantiation that checks non-test fileset for System.out & co). Separately we could open an issue to deal with this virtual problem? It just means our checker isn't as thorough as it is, but none of our checkers/tests are perfect. This all assumes Uwe doesn't wake up with a great solution
          Hide
          Uwe Schindler added a comment -

          In addition, this would work for static field access like System.out or System.err

          Show
          Uwe Schindler added a comment - In addition, this would work for static field access like System.out or System.err
          Hide
          Dawid Weiss added a comment -

          This will quickly grow hairy for more complex type hierarchy checks I think. Not that it isn't possible.

          Show
          Dawid Weiss added a comment - This will quickly grow hairy for more complex type hierarchy checks I think. Not that it isn't possible.
          Hide
          Uwe Schindler added a comment - - edited

          Hi Robert,
          here the patch that implements this. You can add the remaining fields and commit if you like.

          There is one general problem with adding deprecations in general (when doing byte-code analysis):

          In bytecode, we only know two things:

          • The type of field or local variable or static class, on which we do the invoke/putfield/getfield, but we don't know any parent class (in ASM this is called "owner").
          • the name and signature of the method/field to call/getfield/putfield.

          So if you add java.lang.Throwable#printStackTrace() to the list, it will only find invocations of local variables/fields of type java.lang.Throwable. So code like:

          Exception e = .... / } catch (IOException e) {
          e.printStackTrace()
          

          will in bytecocde have no reference to java.lang.Throwable. This would need more work. I will sleep a night about it...

          Show
          Uwe Schindler added a comment - - edited Hi Robert, here the patch that implements this. You can add the remaining fields and commit if you like. There is one general problem with adding deprecations in general (when doing byte-code analysis): In bytecode, we only know two things: The type of field or local variable or static class, on which we do the invoke/putfield/getfield, but we don't know any parent class (in ASM this is called "owner"). the name and signature of the method/field to call/getfield/putfield. So if you add java.lang.Throwable#printStackTrace() to the list, it will only find invocations of local variables/fields of type java.lang.Throwable. So code like: Exception e = .... / } catch (IOException e) { e.printStackTrace() will in bytecocde have no reference to java.lang.Throwable. This would need more work. I will sleep a night about it...

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development