ConcurrentModificationException

[JerryDevis]

Describe the bug

JavaLogFactory.java's readLoggerConfiguration function throws ConcurrentModificationException at this line

if (System.getProperties().keySet().stream().anyMatch(propKey ->
 "java.util.logging.config.class".equals(propKey) || "java.util.logging.config.file".equals(propKey)))

in a multi-threaded environment when another thread is concurrently setting system properties while ESAPI is iterating over system properties.

Specify what ESAPI version(s) you are experiencing this bug in

2.5.2.0

To Reproduce

public class Reproduce {
 public static void main(String[] args) {
 Thread t = new Thread(new Runnable() {
 public void run() {
 while (true) {
 long time = System.currentTimeMillis();
 // Emulate another thread modifying system properties
 System.setProperty(time + "", time + "");
 try {
 Thread.sleep(500);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 }
 }
 }); t.start();
 EsapiThread esapiThread = new EsapiThread();
 esapiThread.start();
 }

 private static class EsapiThread extends Thread {
 public void run() {
 // Emulate the code of JavaLogFactory in ESAPI
 System.getProperties().keySet().stream().anyMatch(a -> {
 try {
 Thread.sleep(2000);
 } catch (InterruptedException e) {
 throw new RuntimeException(e);
 }
 return "dummy".equals(a);
 });
 }
 }
}

Expected behavior

Proposed change: copy the System.getProperties().keySet() to a new collection and iterating over the new collection instead of the original one to avoid thread safety problem.

Platform environment (please complete the following information)

• JDK version: JDK8

[kwwall]

Now that is how you write a bug report. Kudos to you @JerryDevis.

[kwwall]

@JerryDevis - I guess I really should have asked first, but I really thought that you deserved a shout-out for this, so I gave you one on a LinkedIn post. Seriously, many people would have stopped at "I was using 2.5.2.0 and received a ConcurentModificationException" and given no further details. (I'm serious! Just ask @xeno6696 or @jeremiahjstacey.) So this is so refreshing to me, I just had to call you out as a good example. I hope you don't mind, but if you do, let me know and I'll delete it. But I figured it would be better hearing it from me than hearing it secondhand. Thanks again.

[JerryDevis]

Thank you so much @kwwall for the quick reply. I can't open the link of the post you mentioned, but I believe that's no problem. I am trying to give as much detail as I can so that you can see the problem I encountered. I am looking forward to the conclusion of this issue from you/your team.

[kwwall]

@JerryDevis - Well, I appreciate the details that you put into it. A screen shot of the LinkedIn post is attached if you want to see what I posted.

@jeremiahjstacey - since this is in your code (JavaLogFactory) do you want to take a crack at a PR to fix this?

[jeremiahjstacey]

@kwwall The way I would want to fix this would be to remove the custom property file that I used before I understood the java LogManager configuration.

Would we correct the ConcurrentMod by copying the keySet? Yes, absolutely. The problem is that this bug is actually only identifying 1/2 of the problem.
The next bug we can expect is related to the race condition that's causing this exception.
In some environment the JavaLogFactory will not respect the underlying LogManager configuration and possibly alter the jvm log behavior with the esapi-java-logging.properties (which would be equally undesired).

So the question I have is how would you like to see this corrected?

Option 1: Only Fix the ConcurrentModificationException listed in this bug and wait for the runtime exception to be filed later

Option 2: Remove the esapi-java-logging.properties file and only support the javadocs on the java LogManager class.

Option 2 is arguably "better", but will come with the expected feedback when the release drops and no one reads the release notes on the required configuration updates.

I suppose there is a third option to double-down on the custom configuration and add support for observing changes to System Properties at runtime to account for the possible race condition on initialization. I'm not particularly fond of this option, but it is also a path that could be sustained.

[kwwall]

@jeremiahjstacey - I don't like Option 1 because I think the race condition issue that pops up could be way more subtle. A race condition may not even be noticed because unlike the ConcurrentModificationException that gets thrown, it is not going to stop you dead in your tracks. And just because it hasn't been reported doesn't mean it's never happened to anyone. I think it likely has happened, but no one has ever noticed. Because the tendency is to check the logs once and then never look at them again unless something goes wrong and you need to troubleshoot something.

But because you are spot-on regarding your observation to ESAPI users apparent aversion to read ESAPI release notes, I don't think that Option 2 goes far enough and I really don't want to set ourselves up to having users pose endless questions on Stack Overflow, on our Discussions list, as GH issues, and personal emails.

But I also don't like the idea of observing changes to the System Properties to deal with this. So I'm thinking of an alternate approach. What if, upon initialization, you look to see if an esapi-java-logging.properties file exists, and if you find one, we throw a ConfigurationException with an error message that has a short error message and then refers them to a URL on our GitHub wiki for some TBD wiki page where you provide all the details of how they can fix it, one step of which would be to ultimately delete their local esapi-java-logging.properties file. That way (assuming the follow all the detailed steps), we stop them once, they read and follow the instructions, and the restart their application server and the problem goes away. Of course we will write appropriate release notes as well, but with an approach like this, it hopefully eliminate that need to share the fact that they ignored reading the release notes via email, GH issues, Discussions, etc. What do you think of that proposal?

[jeremiahjstacey]

I think that works. The changes I think that implies would be:

• esapi-java-logging.properties files are removed from the baseline and configuration delivery
• The static method behavor in JavaLogFactory readLoggerConfiguration is updated to check if an esapi-java-logging.properties file is provided. If it is specified, then a ConfigurationException will be thrown with a message akin to:

Unsupported esapi-java-logging.properties file found on path. To configure the JavaLogFactory for ESAPI configure the java.util.logging.LogManager instance directly by specifying '-Djava.util.logging.config.file=path/to/configuration.properties'.
An Application Restart may be required for changes to take effect.

Refer to the java LogManager API documentation for more information on how to customize and configure Java Logging for the application.
References: Github Issue 839,

Am I on the same page with this understanding?

[kwwall]

Yeah, that's pretty much it, but for most of the details of what they need to do, I would have the ConfigurationException message refer them to some new page over on our local GH wiki, at https://github.com/ESAPI/esapi-java-legacy/wiki. The advantage of that is we can make further changes / clarifications as needed there without having to update the release. For example if a new future Jakarta release decided to (for some insane reason) rename the JUL property from 'java.util.logging.config.file' to (say) 'jakarta.util.logging.config.file', we would not have to do a new release to account for that. Also having a wiki page gives us a short way to still respond to everyone who still persist on asking about it. But yeah, I think we're on the same page of understanding.

[jeremiahjstacey]

https://github.com/ESAPI/esapi-java-legacy/wiki/Configuring-the-JavaLogFactory

Generated, and stub content is provided based on previous comment.