Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LOG4J2-3228 - Remove support for java.io.Serializable #1199

Merged
merged 4 commits into from Jan 14, 2023

Conversation

jvz
Copy link
Member

@jvz jvz commented Jan 11, 2023

This removes Serializable from various APIs including Message, Layout, LogEvent, Logger, and ReadOnlyStringMap. Java serialization is a security hazard. Messages and LogEvents already have numerous alternatives for generic serialization which should be used instead.

This removes Serializable from various APIs including Message, Layout, LogEvent, Logger, and ReadOnlyStringMap. Java serialization is a security hazard. Messages and LogEvents already have numerous alternatives for generic serialization which should be used instead.
@jvz jvz added the java Pull requests that update Java code label Jan 11, 2023
@jvz
Copy link
Member Author

jvz commented Jan 13, 2023

Alright, @garydgregory, I've removed the unnecessary generic parameter. However, this raises a cleanup issue: we now have AbstractLayout and AbstractStringLayout which logically should be merged, but this isn't really that easy to do without making weird classes like AbstractStringLayout.Serializer2 or breaking backward compatibility. Given the existing tests we have for EcsLayout, there's clearly some desire for binary compatibility with basic Log4j2 plugins, and the abstract layout classes are seemingly one of those. What do you think? Should I try to pull up whatever members we can that maintain binary compatibility, or should I just leave it as is?

@jvz jvz added this to the 3.0.0 milestone Jan 13, 2023
Also removes generic parameter from Layout as it's no longer useful

Signed-off-by: Matt Sicker <mattsicker@apache.org>
@jvz jvz added the api Affects the public API label Jan 14, 2023
import java.util.Objects;

/**
* The internal representation of caller location information.
*
* @since 0.8.3
*/
public class LocationInfo implements Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we ca remove these, since we need to maintain compatibility with Log4j 1.2.

@@ -115,28 +108,14 @@ private boolean equalObjectsOrStrings(final Object left, final Object right) {

@Override
public int hashCode() {
return obj != null ? obj.hashCode() : 0;
return obj.hashCode();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we certain nobody calls new ObjectMessage(null)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    public ObjectMessage(final Object obj) {
        this.obj = obj == null ? "null" : obj;
    }

Doesn't matter if they do.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I scrolled through about half of the modified files, but it looks good to me.

@jvz
Copy link
Member Author

jvz commented Jan 14, 2023

I'm currently running the tests to make sure the revert of log4j-1.2-api code is ok with the rest of this PR before I push that up. Then I can merge from there after a PR run.

Signed-off-by: Matt Sicker <mattsicker@apache.org>
@jvz jvz merged commit c3276a6 into apache:master Jan 14, 2023
@jvz jvz deleted the dev-master-LOG4J2-3228 branch January 14, 2023 23:18
@ppkarwasz ppkarwasz modified the milestones: 3.0.0, 3.0.0-beta1 Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API java Pull requests that update Java code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants