Skip to content
This repository was archived by the owner on Sep 23, 2025. It is now read-only.

fix[log]: log4j with incorrect configuration#6822

Merged
Mathias-Boulay merged 4 commits intoPojavLauncherTeam:v3_openjdkfrom
TarikBR:fix-log4j-configuration
Apr 18, 2025
Merged

fix[log]: log4j with incorrect configuration#6822
Mathias-Boulay merged 4 commits intoPojavLauncherTeam:v3_openjdkfrom
TarikBR:fix-log4j-configuration

Conversation

@TarikBR
Copy link
Copy Markdown
Contributor

@TarikBR TarikBR commented Apr 12, 2025

From what I've tested and observed, this problem occurs more frequently in recent versions of Minecraft

So, following the idea of some forks, like Zalith, I did a check using DateUtils to detect if the minecraft version is older than 1.12, if it is it uses the 1.7 configuration of log4j, if it is 1.12 or higher, it uses the 1.12 configuration

Created a new config for minecraft 1.21.2+, with the additions from minecraft's one, and made a method to unpack this new config in an existing installation

@TarikBR
Copy link
Copy Markdown
Contributor Author

TarikBR commented Apr 12, 2025

Now I don't know if this would be the best method, or if it would be better to create a new config for 1.21.2, which is where part of the config was changed, so pojav wouldn't select any of these settings

If I were to create a new version config, it would be as follows, with the additions of the new minecraft configuration

log4j-rce-patch-1.21.2xml

<?xml version="1.0" encoding="UTF-8"?>
<Configuration status="WARN">
    <Appenders>
        <Console name="SysOut" target="SYSTEM_OUT">
            <PatternLayout pattern="[%d{HH:mm:ss}] [%t/%level]: %msg{nolookups}%n" />
            <Policies>
                <TimeBasedTriggeringPolicy />
                <OnStartupTriggeringPolicy />
            </Policies>
        </Console>
        <RollingRandomAccessFile name="File" fileName="logs/latest.log" filePattern="logs/%d{yyyy-MM-dd}-%i.log.gz">
            <PatternLayout pattern="[%d{HH:mm:ss}] [%t/%level]: %msg{nolookups}%n" />
            <Policies>
                <TimeBasedTriggeringPolicy />
                <OnStartupTriggeringPolicy />
            </Policies>
        </RollingRandomAccessFile>
+       <Listener name="Tracy">
+           <PatternLayout pattern="(%F:%L): %msg{nolookups}%n"/>
+       </Listener>
    </Appenders>
    <Loggers>
        <Root level="info">
            <filters>
                <MarkerFilter marker="NETWORK_PACKETS" onMatch="DENY" onMismatch="NEUTRAL" />
            </filters>
            <AppenderRef ref="SysOut"/>
            <AppenderRef ref="File"/>
+           <AppenderRef ref="Tracy"/>
        </Root>
    </Loggers>
</Configuration>

@TarikBR
Copy link
Copy Markdown
Contributor Author

TarikBR commented Apr 12, 2025

Now added new configuration, and made a method to unpack this new config in an existing installation

@crystall1nedev
Copy link
Copy Markdown
Member

From what I've tested and observed, this problem occurs more frequently in recent versions of Minecraft

What is the issue?

@TarikBR
Copy link
Copy Markdown
Contributor Author

TarikBR commented Apr 14, 2025

From what I've tested and observed, this problem occurs more frequently in recent versions of Minecraft

What is the issue?

the log looks like this, without the correct formatting
image

@crystall1nedev
Copy link
Copy Markdown
Member

I see, looks good to me in terms of getting the log fixed.

Copy link
Copy Markdown
Contributor

@Mathias-Boulay Mathias-Boulay left a comment

Choose a reason for hiding this comment

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

See comments, nothing much, but it has to be addressed.

private static String getLog4jConfiguration(Context ctx, JMinecraftVersionList.LoggingConfig loggingConfig){
String configFilePath = Tools.DIR_DATA + "/security/" + loggingConfig.client.file.id.replace("client", "log4j-rce-patch");
File configFile = new File(configFilePath);
if (!configFile.exists()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The outer if statement is redundant. copyAssetFile already checks for the presence of the file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is that better?

    private static String getLog4jConfiguration(Context ctx, JMinecraftVersionList.LoggingConfig loggingConfig){
        String configFilePath = Tools.DIR_DATA + "/security/" + loggingConfig.client.file.id.replace("client", "log4j-rce-patch");
        File configFile = new File(configFilePath);

        try {
            copyAssetFile(ctx,"components/security/" + loggingConfig.client.file.id.replace("client", "log4j-rce-patch"), Tools.DIR_DATA + "/security", false);
        } catch (IOException e) {
            Log.w("log4j-unpack", e.getMessage(), e);
        }

        if (!configFile.exists()) {
            configFilePath = Tools.DIR_GAME_NEW + "/" + loggingConfig.client.file.id;
        }
        return configFilePath;
    }

// try unpacking a new configuration from an existing installation
try {
copyAssetFile(ctx,"components/security/" + loggingConfig.client.file.id.replace("client", "log4j-rce-patch"), Tools.DIR_DATA + "/security", false);
} catch (IOException ignored) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't. If you intend to ignore an exception, it should at least be logged as a warning in the catch clause.

@TarikBR TarikBR requested a review from Mathias-Boulay April 18, 2025 02:36
Copy link
Copy Markdown
Contributor

@Mathias-Boulay Mathias-Boulay left a comment

Choose a reason for hiding this comment

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

Forgot an important point during my last review, see the comment 😅

String configFilePath = Tools.DIR_DATA + "/security/" + loggingConfig.client.file.id.replace("client", "log4j-rce-patch");
File configFile = new File(configFilePath);

// try unpacking the config (useful for new configurations in existing installations)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now, I may have forgotten to tell you that, but unpacking like this on the fly is not the way to do it.

Instead, you should rely on the assets/components/security/version file. By updating the version, the content of the security folder will get updated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

huh, i forgot that version file also, i don't know why i complicated something simple

@TarikBR TarikBR requested a review from Mathias-Boulay April 18, 2025 17:49
@Mathias-Boulay Mathias-Boulay merged commit 42c1c98 into PojavLauncherTeam:v3_openjdk Apr 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants