Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-1926

Remove dependency on RMI and Management APIs from log4j-api

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8
    • Fix Version/s: 2.9.0
    • Component/s: API
    • Labels:
    • Environment:

      Android

      Description

      (Remko: Paraphrasing discussion on the log4j dev mailing list. Please feel free to update/modify):

      When the Apache HttpClient 5.0 library gets pulled into an Android project, the Lint static code analyzer reports two severe violations due to transitive dependency through Log4j APIs 2.8 on Java RMI and Java Management APIs.

      At the moment adding a transitive dependency on log4j2-api causes any Android build to fail with a scary invalid package error. Surely this error can be ignored with a custom lint rule but it may present a certain reason for concert to less experienced developers.

      This is caused by Log4j's use of MarshalledObject: User domain objects and exceptions are wrapped in MarshalledObject when LogEvents are serialized. This allows applications like Lilith to deserialize LogEvents even when not all domain classes are on the classpath (LOG4J2-1226).

      Consider finding a different way to solve this problem that does not require MarshalledObject.

        Issue Links

          Activity

          Hide
          remkop@yahoo.com Remko Popma added a comment -

          Oleg Kalnichevski you mentioned there were 2 dependencies causing warnings in log4j-api. We talked about java.rmi.MarshalledObject imported by SortedArrayStringMap, what is the 2nd one? (Error message text may be useful.)

          Show
          remkop@yahoo.com Remko Popma added a comment - Oleg Kalnichevski you mentioned there were 2 dependencies causing warnings in log4j-api. We talked about java.rmi.MarshalledObject imported by SortedArrayStringMap, what is the 2nd one? (Error message text may be useful.)
          Hide
          olegk Oleg Kalnichevski added a comment -

          o.a.l.log4j.message.ExtendedThreadInformation imports java.lang.management classes thus making log4j-api dependent on JMX APIs. JMX is not supported by Android.

          Oleg

          Show
          olegk Oleg Kalnichevski added a comment - o.a.l.log4j.message.ExtendedThreadInformation imports java.lang.management classes thus making log4j-api dependent on JMX APIs. JMX is not supported by Android. Oleg
          Hide
          remkop@yahoo.com Remko Popma added a comment - - edited

          Ah, thanks. I see it now.
          That one is trickier as it gets information from the platform MBeans we can't get elsewhere.

          Not sure how to go about this one. Use reflection to avoid compile time dependency on java.lang.management? Then at runtime it would simply provide less information on Android. This would work but has the usual drawbacks of using reflection.

          Does anyone have ideas for a different approach?

          Show
          remkop@yahoo.com Remko Popma added a comment - - edited Ah, thanks. I see it now. That one is trickier as it gets information from the platform MBeans we can't get elsewhere. Not sure how to go about this one. Use reflection to avoid compile time dependency on java.lang.management? Then at runtime it would simply provide less information on Android. This would work but has the usual drawbacks of using reflection. Does anyone have ideas for a different approach?
          Hide
          olegk Oleg Kalnichevski added a comment -

          Remko,
          The real question to me is whether or not ThreadDumpMessage (and similar) should have been made a part of log4-api in the first place. If you are not willing to re-visit that decision, I am afraid reflection is the only way to avoid direct compile-time dependency on external APIs I personally know of.

          Oleg

          Show
          olegk Oleg Kalnichevski added a comment - Remko, The real question to me is whether or not ThreadDumpMessage (and similar) should have been made a part of log4-api in the first place. If you are not willing to re-visit that decision, I am afraid reflection is the only way to avoid direct compile-time dependency on external APIs I personally know of. Oleg
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          In SortedArrayStringMap we can replace MarshalledObject by a simple byte[] array representing the marshalled data. That will remove the dependency on the java.rmi package.

          To remove the dependency on the java.lang.management package in ThreadDumpMessage and ExtendedThreadInformation there are a few options:

          1. Use reflection when using the classes in java.lang.management. On Android, only provide BasicThreadInfo.
          2. Move ThreadDumpMessage out of log4j-api entirely.
          3. Keep ThreadDumpMessage in log4j-api but reduce functionality to always only provide BasicThreadInfo. Delete ExtendedThreadInformation.
          4. Keep ThreadDumpMessage in log4j-api but move ExtendedThreadInformation and other logic that uses java.lang.management into log4j-core. Provide some mechanism (service provider?) to load this functionality from log4j-core.

          I suspect it will be hard to convince anyone in the Log4j 2 community to break binary compatibility in the log4j-api so I doubt that option 2 is realistic. 3 seems a bit last resort. 1 or 4 seem doable. Thoughts anyone?

          Show
          remkop@yahoo.com Remko Popma added a comment - In SortedArrayStringMap we can replace MarshalledObject by a simple byte[] array representing the marshalled data. That will remove the dependency on the java.rmi package. To remove the dependency on the java.lang.management package in ThreadDumpMessage and ExtendedThreadInformation there are a few options: Use reflection when using the classes in java.lang.management. On Android, only provide BasicThreadInfo. Move ThreadDumpMessage out of log4j-api entirely. Keep ThreadDumpMessage in log4j-api but reduce functionality to always only provide BasicThreadInfo. Delete ExtendedThreadInformation. Keep ThreadDumpMessage in log4j-api but move ExtendedThreadInformation and other logic that uses java.lang.management into log4j-core. Provide some mechanism (service provider?) to load this functionality from log4j-core. I suspect it will be hard to convince anyone in the Log4j 2 community to break binary compatibility in the log4j-api so I doubt that option 2 is realistic. 3 seems a bit last resort. 1 or 4 seem doable. Thoughts anyone?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 194a398d8f70d6680f8154168011171f0a12113b in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=194a398 ]

          LOG4J2-1926 removed dependency on java.rmi for Android compatibility

          Show
          jira-bot ASF subversion and git services added a comment - Commit 194a398d8f70d6680f8154168011171f0a12113b in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=194a398 ] LOG4J2-1926 removed dependency on java.rmi for Android compatibility
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f23fd6e4bf537fd6adec8ab3a62929cdb05ad389 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=f23fd6e ]

          LOG4J2-1926 prevent NPE when serializing null values

          Show
          jira-bot ASF subversion and git services added a comment - Commit f23fd6e4bf537fd6adec8ab3a62929cdb05ad389 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=f23fd6e ] LOG4J2-1926 prevent NPE when serializing null values
          Hide
          mikaelstaldal Mikael Ståldal added a comment -

          Remko Popma, why did we use MarshalledObject in the first place? Why not simply serialize the values, rather than wrapping them? I think that this wrapping may open a Serialization vulnerability, since the FilteredObjectInputStream is not used for the wrapped value.

          Show
          mikaelstaldal Mikael Ståldal added a comment - Remko Popma , why did we use MarshalledObject in the first place? Why not simply serialize the values, rather than wrapping them? I think that this wrapping may open a Serialization vulnerability, since the FilteredObjectInputStream is not used for the wrapped value.
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          MarshalledObjects in the API module have been replaced by byte[] arrays in the commits referenced above.

          What remains is the dependency on the java.lang.management package in ThreadDumpMessage and ExtendedThreadInformation.

          I offered some ideas on removing these dependencies in a comment above. Feedback is welcome.

          Show
          remkop@yahoo.com Remko Popma added a comment - MarshalledObjects in the API module have been replaced by byte[] arrays in the commits referenced above. What remains is the dependency on the java.lang.management package in ThreadDumpMessage and ExtendedThreadInformation . I offered some ideas on removing these dependencies in a comment above. Feedback is welcome.
          Hide
          remkop@yahoo.com Remko Popma added a comment - - edited

          Mikael, sorry, I misunderstood your concern.
          Please see LOG4J2-1663 and LOG4J2-1226 for the background.
          Basically this allows applications like Lilith to deserialize LogEvents even when the event contains domain objects that are not in Lilith's classpath when derserializing.

          It's a good idea to use the FilteredObjectInputStream instead of the JDK ObjectInputStream when deserializing, and the recent changes should make this easier to accomplish.

          Show
          remkop@yahoo.com Remko Popma added a comment - - edited Mikael, sorry, I misunderstood your concern. Please see LOG4J2-1663 and LOG4J2-1226 for the background. Basically this allows applications like Lilith to deserialize LogEvents even when the event contains domain objects that are not in Lilith's classpath when derserializing. It's a good idea to use the FilteredObjectInputStream instead of the JDK ObjectInputStream when deserializing, and the recent changes should make this easier to accomplish.
          Hide
          mikaelstaldal Mikael Ståldal added a comment -

          OK, I see now why we wrap the values.

          But I think that the current way you wrap it will open the serialization security vulnerability CVE-2017-5645 we fixed with FilteredObjectInputStream in LOG4J2-1863 since the nested ObjectInputStream does not respect the filtering.

          And we cannot simply use a nested FilteredObjectInputStream since that class is not in log4j-api, and even if it were it might be tricky to know which classes to filter. (I don't think it would be wise to move FilteredObjectInputStream to log4j-api).

          And BTW, doesn't e.g. ObjectMessage (which does not do this wrapping) break Lilith? Is LOG4J2-1226 really fixed?

          Show
          mikaelstaldal Mikael Ståldal added a comment - OK, I see now why we wrap the values. But I think that the current way you wrap it will open the serialization security vulnerability CVE-2017-5645 we fixed with FilteredObjectInputStream in LOG4J2-1863 since the nested ObjectInputStream does not respect the filtering. And we cannot simply use a nested FilteredObjectInputStream since that class is not in log4j-api, and even if it were it might be tricky to know which classes to filter. (I don't think it would be wise to move FilteredObjectInputStream to log4j-api). And BTW, doesn't e.g. ObjectMessage (which does not do this wrapping) break Lilith? Is LOG4J2-1226 really fixed?
          Hide
          mikaelstaldal Mikael Ståldal added a comment -

          If we cannot come up with an reasonably simple and secure solution to this, maybe we should just mark LOG4J2-1663 and LOG4J2-1226 as WONTFIX since we plan to deprecate SerializedLayout in 2.9?

          Show
          mikaelstaldal Mikael Ståldal added a comment - If we cannot come up with an reasonably simple and secure solution to this, maybe we should just mark LOG4J2-1663 and LOG4J2-1226 as WONTFIX since we plan to deprecate SerializedLayout in 2.9?
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          Getting back to the topic of this ticket, I have given it some more thought and I think the following would be a clean solution:

          • move ExtendedThreadInformation to log4j-core
          • make inner interface ThreadDumpMessage.ThreadInfoFactory public
          • make inner class ThreadDumpMessage.ExtendedThreadInfoFactory a top-level class and move it to log4j-core
          • change ThreadDumpMessage to use the standard service provider extension mechanism to look up the ThreadInfoFactory. If ExtendedThreadInfoFactory cannot be found or instantiated, fall back to BasicThreadInfoFactory, like we do now.

          This will push the dependency on java.lang.management out of log4j-api and into log4j-core. That should solve the remainder of the problems with using log4j-api on Android.

          Show
          remkop@yahoo.com Remko Popma added a comment - Getting back to the topic of this ticket, I have given it some more thought and I think the following would be a clean solution: move ExtendedThreadInformation to log4j-core make inner interface ThreadDumpMessage.ThreadInfoFactory public make inner class ThreadDumpMessage.ExtendedThreadInfoFactory a top-level class and move it to log4j-core change ThreadDumpMessage to use the standard service provider extension mechanism to look up the ThreadInfoFactory. If ExtendedThreadInfoFactory cannot be found or instantiated, fall back to BasicThreadInfoFactory, like we do now. This will push the dependency on java.lang.management out of log4j-api and into log4j-core. That should solve the remainder of the problems with using log4j-api on Android.
          Hide
          garydgregory Gary Gregory added a comment -

          How would you document the BC breaks?

          Gary

          Show
          garydgregory Gary Gregory added a comment - How would you document the BC breaks? Gary
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          I don't think there will be changes that break binary compatibility: the classes I intend to move out of log4j-api into log4j-core are currently either package private (ExtendedThreadInformation) or private inner classes (ThreadDumpMessage.ExtendedThreadInfoFactory).

          Show
          remkop@yahoo.com Remko Popma added a comment - I don't think there will be changes that break binary compatibility: the classes I intend to move out of log4j-api into log4j-core are currently either package private (ExtendedThreadInformation) or private inner classes (ThreadDumpMessage.ExtendedThreadInfoFactory).
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 000899acba05e5b9a2a630d59164ba055553e278 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=000899a ]

          LOG4J2-1926 make interface ThreadInformation public since some implementations will live in log4j-core

          Show
          jira-bot ASF subversion and git services added a comment - Commit 000899acba05e5b9a2a630d59164ba055553e278 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=000899a ] LOG4J2-1926 make interface ThreadInformation public since some implementations will live in log4j-core
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d0023dee94ecee680ee3cdc0be31313337fd49ad in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=d0023de ]

          LOG4J2-1926 Remove references to java.lang.management package

          • make inner interface ThreadInfoFactory public
          • obtain ThreadInfoFactory instance from ServiceLoader, fall back to BasicThreadInfoFactory
          • move ExtendedThreadInfoFactory out of ThreadDumpMessage class
          Show
          jira-bot ASF subversion and git services added a comment - Commit d0023dee94ecee680ee3cdc0be31313337fd49ad in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=d0023de ] LOG4J2-1926 Remove references to java.lang.management package make inner interface ThreadInfoFactory public obtain ThreadInfoFactory instance from ServiceLoader, fall back to BasicThreadInfoFactory move ExtendedThreadInfoFactory out of ThreadDumpMessage class
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 75f519f75cdd576b264c05af3d4608d6cb8d2f97 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=75f519f ]

          LOG4J2-1926 moved ExtendedThreadInfoFactory out of ThreadDumpMessage class and make it a top-level class in log4j-core

          Show
          jira-bot ASF subversion and git services added a comment - Commit 75f519f75cdd576b264c05af3d4608d6cb8d2f97 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=75f519f ] LOG4J2-1926 moved ExtendedThreadInfoFactory out of ThreadDumpMessage class and make it a top-level class in log4j-core
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 98eaaf9861156661f8d053adced0856fb7b54d6b in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=98eaaf9 ]

          LOG4J2-1926 Facilitate Adroid use: Moved ExtendedThreadInformation from log4j-api to log4j-core to avoid all references to java.lang.management in log4j-api

          Show
          jira-bot ASF subversion and git services added a comment - Commit 98eaaf9861156661f8d053adced0856fb7b54d6b in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=98eaaf9 ] LOG4J2-1926 Facilitate Adroid use: Moved ExtendedThreadInformation from log4j-api to log4j-core to avoid all references to java.lang.management in log4j-api
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 20dab46a8dda6c42c0a0d91085154652a2eaac1a in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=20dab46 ]

          LOG4J2-1926 added configuration file to obtain ExtendedThreadInfoFactory instance from ServiceLoader

          Show
          jira-bot ASF subversion and git services added a comment - Commit 20dab46a8dda6c42c0a0d91085154652a2eaac1a in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=20dab46 ] LOG4J2-1926 added configuration file to obtain ExtendedThreadInfoFactory instance from ServiceLoader
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e369b8a51fe7adec9afb58a42d279d9d41a08370 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=e369b8a ]

          LOG4J2-1926 unit test that verifies ExtendedThreadInfoFactory is loaded from ServiceLoader

          Show
          jira-bot ASF subversion and git services added a comment - Commit e369b8a51fe7adec9afb58a42d279d9d41a08370 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=e369b8a ] LOG4J2-1926 unit test that verifies ExtendedThreadInfoFactory is loaded from ServiceLoader
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 81f20a6accf14c1c0eb058d4d4c5579cbf4bdec3 in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=81f20a6 ]

          LOG4J2-1926 change log entry

          Show
          jira-bot ASF subversion and git services added a comment - Commit 81f20a6accf14c1c0eb058d4d4c5579cbf4bdec3 in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=81f20a6 ] LOG4J2-1926 change log entry
          Hide
          remkop@yahoo.com Remko Popma added a comment -

          The changes described in my previous comment are now pushed to master.
          I believe that eliminates all references to RMI or Management APIs from log4j-api.

          Please verify and close.

          Show
          remkop@yahoo.com Remko Popma added a comment - The changes described in my previous comment are now pushed to master. I believe that eliminates all references to RMI or Management APIs from log4j-api. Please verify and close.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 69d7e421c64b0fce53cbb08d4c1da890a0f395bd in logging-log4j2's branch refs/heads/master from rpopma
          [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=69d7e42 ]

          LOG4J2-1926 document that implementations of the ThreadDumpMessage$ThreadInfoFactory interface are loaded via the Service Loader Interface

          Show
          jira-bot ASF subversion and git services added a comment - Commit 69d7e421c64b0fce53cbb08d4c1da890a0f395bd in logging-log4j2's branch refs/heads/master from rpopma [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=69d7e42 ] LOG4J2-1926 document that implementations of the ThreadDumpMessage$ThreadInfoFactory interface are loaded via the Service Loader Interface
          Hide
          olegk Oleg Kalnichevski added a comment -

          Android static code analyzer no longer chokes on log4j-api. Tested with the latest 2.8.3-SNAPSHOT.

          Oleg

          Show
          olegk Oleg Kalnichevski added a comment - Android static code analyzer no longer chokes on log4j-api. Tested with the latest 2.8.3-SNAPSHOT. Oleg

            People

            • Assignee:
              remkop@yahoo.com Remko Popma
              Reporter:
              olegk Oleg Kalnichevski
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development