search

openjump-gis / openjump

OpenJUMP, the Open Source GIS with more than one trick in its kangaroo pocket, takes the leap from svn to git. join the effort!
http://openjump.org
GNU General Public License v2.0
28 stars 14 forks source link

Replacing log4j 1 with a modern, supported logging framework #49

Open Neutius opened 2 years ago

Neutius commented 2 years ago

OpenJUMP is currently still using log4j 1 as a logging implementation. This logging implementation has been end of life and unsupported since 2015. The recently published security vulnerability probably doesn't affect OpenJUMP at all, but it might still be a good reminder that outdated dependencies might pose a risk.

Me and my team would like to donate some of our time to help remove log4j 1 from OpenJUMP. Our company has been using OpenJUMP for years. We have recently removed log4j 1 from our own legacy application and we'd like to volunteer our newly gained experience.

The direct usage of log4j 1 code in the OpenJUMP code base is mostly isolated/concentrated in the com.vividsolutions.jump.workbench.Logger class. There are 6 other classes handling (part of) their own logging, which should probably all be using the centralized Logger class instead. The raw amount of work seems pretty limited.

In general, it's best practice to use a logging API like Apache Commons Logging or SLF4J. There's a TODO comment above the Logger class mentioning Apache Commons Logging, my team has more experience with SLF4J, but I'm sure either one is perfectly fine.

However, the Logger class does directly manipulate and query the logging implementation. We have dealt with a similar situation, and found it unfeasible to do this via SLF4J. Our solution was to use log4j 2 (which is still being supported) and manipulate that logging implementation instead.

Should we investigate the possibility of using SLF4J or Apache Commons Logging in OpenJUMP? Or would it be enough of an improvement to upgrade the logging implementation to log4j 2?

edeso commented 2 years ago

hey Neutius,

thanks for the generous offer. any contribution is greatly appreciated.

totally agree with your assessment above. everything should use the central Logger, which in turn bundles the implementation details for whichever logger implementation is actually used.

needs we had for now were Log Levels, Console and Logfile Printing. nothing fancy really. ignore the comment from 2016, as times have changed and implementations with it.

reads as if the advantage of using SLF4J for API separation disappeared, which was probably the reason for the Commons-Logging comment as well https://stackoverflow.com/questions/41498021/is-it-worth-to-use-slf4j-with-log4j2/41500347#41500347

but as we separate API via Logger already, every solution fitting in there should do the trick. we particularily try not to pull in to big dependencies, so maybe that is something to look out for as well.

jratike80 commented 2 years ago

Geoserver is just changing away from Log4J 1 by this plan https://github.com/geoserver/geoserver/wiki/GSIP-167. We did not make a large evaluation about the alternatives but we did find this comparison table http://www.java-logging.com/comparison/ but log4j2 was selected because a) we got a maker for log4j2 switch and b) I think we was shown results about log4j2 being much faster in some use scenario of Geoserver. I wonder what it was, this table shows different numbers https://logback.qos.ch/performance.html but what ever they are I suppose that speed is not critical for OpenJUMP.

One developer was a bit in favor of logback because it lacks features that have made log4j2 vulnerable and because the upgrade from log4j 1 maybe would have been easier.

I found also these planning notes https://github.com/geoserver/geoserver/wiki/Update-or-replace-Log4J-1-library.

Neutius commented 2 years ago

Thanks for the quick responses!

we separate API via Logger already

That's a good point, actually. The current setup already provides a separation of API and implementation, while also allowing the manipulation of the underlying implementation. Introducing SLF4J might even be over-engineering in this case.

I'll discuss with my team and we'll hopefully be able to offer a pull request soon. I'll propose two steps:

  1. Moving all interactions with the logging implementation to the OpenJUMP Logger class.
  2. Upgrading to log4j 2 (while keeping the same behaviour and configuration).
edeso commented 2 years ago
  • Moving all interactions with the logging implementation to the OpenJUMP Logger class.
  • Upgrading to log4j 2 (while keeping the same behaviour and configuration).

sound good to me. thanks again!

edeso commented 2 years ago

hey @Neutius,

thanks for the PR. i'll have a look in the course of the coming week. regards ede

Neutius commented 2 years ago

Hi @edeso,

Thanks, sounds great!

I started work on the second part - the actual upgrade to log4j 2. I hope to open a second PR later this week.

Neutius commented 2 years ago

I'm busy working on the actual upgrade to Log4j2.

I currently have a version that uses both Log4j 1 and 2, using both versions to write to 2 different files and to the same console. Feel free to take a peak or try it out: https://github.com/rigd-loxia/openjump/pull/1

I'm not 100% sure I have maintained the same behaviour between log4j 1 and 2, so I'll look into that some more.

Next step will be to re-write all interactions with Log4j in the com.vividsolutions.jump.workbench.Logger class, e.g. concerning log levels and log files. When that is done, all Log4j 1 stuff can be taken out.

Neutius commented 2 years ago

@edeso I have fully implemented and configured Log4j2 - both Log4j 1 and 2 are being used. See https://github.com/rigd-loxia/openjump/pull/1

I'll remove Log4j 1 next week. Let me know if you'd like to test out the changes, I can keep this version around on a separate branch.

edeso commented 2 years ago

@Neutius no need to keep Log4J v1 around. we can easily compare with older builds when the PR was merged. i'll have a more detailed look then.

thanks!