Skip to content

fix: resolve memory leak in ChallengeConfig and adjust JDK 22 build c… #2467

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.

Sign up for GitHub

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

Open
hemasree1516 wants to merge 4 commits into
base: master
Choose a base branch
from
+11 −3
Open

fix: resolve memory leak in ChallengeConfig and adjust JDK 22 build c… #2467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b69ccb
fix: resolve memory leak in ChallengeConfig and adjust JDK 22 build c…
hemasree1516 Mar 22, 2026
4ca4035
chore: revert Java version to 25 to align with project standards
hemasree1516 Mar 22, 2026
36de8ce
Merge branch 'master' into fix/issue-389-memory-optimization
hemasree1516 Mar 23, 2026
6380d71
Merge branch 'master' into fix/issue-389-memory-optimization
commjoen Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
<!-- Pin Groovy to 4.x for thymeleaf-layout-dialect compatibility (Spring Boot 4.0 manages 5.x) -->
<groovy.version>4.0.25</groovy.version>
<io.netty.version>4.2.10.Final</io.netty.version>
<java.version>25</java.version>
Copy link
Copy Markdown
Collaborator

@commjoen commjoen Mar 22, 2026

Choose a reason for hiding this comment

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

Hi @hemasree1516 thank you for your PR! Question: why do we downgrade to Java 22?

Copy link
Copy Markdown
Author

@hemasree1516 hemasree1516 Mar 22, 2026

Choose a reason for hiding this comment

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

Hi @commjoen , thank you for the feedback! I originally adjusted the version to 22 to resolve some MojoExecutionException issues I encountered in my local Windows environment while testing.

However, I understand the project standard is Java 25. I have just pushed a commit to revert the pom.xml to version 25 while keeping the memory leak fix for #389. Please let me know if the CI passes now or if further adjustments are needed.

<java.version>22</java.version>
<jquery.version>3.7.1</jquery.version>
<jruby.version>10.0.3.0</jruby.version>
<lombok.version>1.18.44</lombok.version>
Expand Down
14 changes: 11 additions & 3 deletions src/main/java/org/owasp/wrongsecrets/definitions/ChallengeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Supplier;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.FilenameUtils;
Expand Down Expand Up @@ -40,12 +42,18 @@ public StringToChallengeNameConverter nameConverter() {
return new StringToChallengeNameConverter();
}

private record TextWithFileLocationConverter(TemplateGenerator templateGenerator)
private static final class TextWithFileLocationConverter
Copy link
Copy Markdown
Collaborator

@nbaars nbaars Mar 24, 2026

Choose a reason for hiding this comment

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

This is not really necessary nested records are implicitly static, basically the same.

Copy link
Copy Markdown
Author

@hemasree1516 hemasree1516 Mar 24, 2026

Choose a reason for hiding this comment

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

Thank you for the feedback, @nbaars! You're absolutely right that nested records are implicitly static.

I refactored the record to a static final class primarily to implement the thread-safe memoization pattern using a ConcurrentHashMap to resolve the memory leak identified in #389. Since records are intended for immutable data and don't allow additional instance fields, the class structure felt more appropriate for maintaining the cache state.

Would you prefer I revert it to a record and try to handle the caching differently (e.g., using a static cache if appropriate), or is this class structure acceptable given the optimization goal?

implements Converter<String, TextWithFileLocation> {
private final TemplateGenerator templateGenerator;
private final Map<String, Supplier<String>> cache = new ConcurrentHashMap<>();

public TextWithFileLocationConverter(TemplateGenerator templateGenerator) {
this.templateGenerator = templateGenerator;
}

@Override
public TextWithFileLocation convert(String source) {
return new TextWithFileLocation(source, read(source));
return new TextWithFileLocation(source, cache.computeIfAbsent(source, this::read));
}

private Supplier<String> read(String name) {
Expand Down Expand Up @@ -78,7 +86,7 @@ public Challenges challenges(
.filter(challenge -> challenge instanceof Challenge8)
.findFirst()
.get()
.spoiler(); // need early init to log the secret for debugging ;-).
.spoiler();
return new Challenges(challengeDefinitions, challenges);
}
}
Loading