A hard -1 Thank you for pitching in but this PR completely breaks major blocks of functionality.
I see comments like "Changed in 1.2.18+ to complain about its use and do nothing else." for the JDBC Appender, Telnet Appender, ExternallyRolledFileAppender, JMS Appender, JMX, and more! 140 files changed? You might want to do one PR per CVE/issue. The PR says "binary compatible" but that's only because the code has been gutted which will break applications left and right. This PR is so far out-of-bounds that I have a hard time finding where to start. 1.x is End-Of-Life. If we do ANYTHING, it would be to address CVEs. We have CVE: CVE-2019-1757. We also have discussed security in the JMS Appender. So that would be two changes to do in two PRs IMO. This PR deals with neither in a meaningful manner. I see a disconnect between the email and the PR: "So as requested I've made a conservative fully binary compatible version of all the build changes and security fixes, patches are on a new pull request at" This PR contains ZERO security fixes, it just removes code. For me "conservative" means the least amount of changes, not gutting the code, changing 140 files, and breaking applications with no alternative. "Trying to have solid network code in 1.x while having it also be binary compatible is way too much work" So you've gutted the code because doing real fixes is "too much work". There you have it I guess. I am not endorsing a 1.2.18 release but if there was consensus on the PMC for resurrection it should be ONLY to address IMO CVE-2019-1757 and the JMS Appender using the same techniques we used in 2.x. I'll leave it to each person to decide what is "too much work", just note that it took 6 of us working almost full time to work on 2.15.0, 2.16.0, 2.17.0, and 2.12.2. Keep in mind that this is a low-level library that must be a drop-in replacement in all settings from the largest enterprise to a hobbyist. Hence our choices of remedies in 2.x. Gary On Sun, Dec 19, 2021 at 8:14 AM Leo Simons <m...@leosimons.com> wrote: > > Hey folks, > > So as requested I've made a conservative fully binary compatible version of > all the build changes and security fixes, patches are on a new pull request > at > > https://github.com/apache/log4j/pull/17 > > On Sat, Dec 18, 2021 at 7:30 PM Gary Gregory <garydgreg...@gmail.com> wrote: > > > Again, you cannot break binary compatibility in a minor release. That's a > > show stopper. > > > > Ok, if that's what you want....hope the new approach is acceptable on > review. > > I will note most of log4j 1.2 was not nearly so strict (semantic versioning > not being a standard 'thing' back then), as you can also tell from the > plans to drop chainsaw from 1.2.18 and so on...but it does make sense as a > policy in 2021 I guess. > > Depending on what the consensus approach eventually becomes, you could also > pick up from an earlier commit, for example taking all the build fixes but > writing your own java code changes. > > > > This discussion IMO should ONLY be about mitigation of CVEs and that means > > porting the idea of the fixes from 2.x to 1.x. This 1.x component is EOL. I > > say "idea" because the code bases for 1.x and 2.x are completely different. > > > > As I wrote before I don't agree with this part of the strategy you propose. > I understand it in the abstract but I do not think it is realistic. 2.x has > a lot of network code that is a lot better and more modern than is in 1.x. > Trying to have solid network code in 1.x while having it also be binary > compatible is way too much work, and even if you manage to then test that > things are behavior-compatible at runtime...there'd then be a real risk of > unintended regressions there. > > I think the mitigation strategy that's on the PR now is good enough(tm) to > help many people. > > From my side, I am now out of time to work on this for the next couple of > weeks. I may try and check my e-mail a bit next week, but no promises. I > hope this is in a good state so others in the community can pick things up > from there! > > > Meanwhile, take care, cheers, > > > Leo
