Skip to content

LOG4J2-3452: Fix race condition in FileUtils.mkdir()#809

Merged
ppkarwasz merged 2 commits intoapache:release-2.xfrom
stefanvodita:release-2.x
Mar 30, 2022
Merged

LOG4J2-3452: Fix race condition in FileUtils.mkdir()#809
ppkarwasz merged 2 commits intoapache:release-2.xfrom
stefanvodita:release-2.x

Conversation

@stefanvodita
Copy link
Copy Markdown
Contributor

A race condition could appear when 2 threads were trying to create the
same directory. Both would check that the directory did not exist, then
one would create the directory and the other would throw an exception.

This was reproduced with a unit test included this commit and fixed by
using Files.createDirectories() instead of File.mkdirs(), so that the
existence check and directory creation are done as an atomic operation.

Note: I didn't manage to run the tests with Maven, but I ran the tests in
FileUtilsTest from IntelliJ. I'll come back to this when I have more time.

A race condition could appear when 2 threads were trying to create the
same directory. Both would check that the directory did not exist, then
one would create the directory and the other would throw an exception.

This was reproduced with a unit test included this commit and fixed by
using Files.createDirectories() instead of File.mkdirs(), so that the
existence check and directory creation are done as an atomic operation.
Comment thread log4j-core/src/test/java/org/apache/logging/log4j/core/util/FileUtilsTest.java Outdated
@ppkarwasz ppkarwasz merged commit 396ec80 into apache:release-2.x Mar 30, 2022
@ppkarwasz
Copy link
Copy Markdown
Contributor

@stefanvodita,
Thank you for your contribution.

@merkisoft
Copy link
Copy Markdown

if the directory exists, it still tries to create it - which breaks when using a policy file - you should have left "if exists just return"

@stefanvodita
Copy link
Copy Markdown
Contributor Author

Just to check that I understand the problem - is there an exception you are encountering, which we could have caught when calling Files.createDirectories(dir.toPath());? If you have a test that reproduces the issue, that would be very helpful.

@merkisoft
Copy link
Copy Markdown

the problem is that althought the logs dir exists, you call to create it and java security manager kicks in and it requires these two lines (i only had the 2nd initially, because in my view log4j should not attempt to write the dir):

permission java.io.FilePermission "${user.dir}/logs", "read,write";    //  <<< this was missing and grants mkdir on the parent
permission java.io.FilePermission "${user.dir}/logs/-", "read,write";

config - using log4j 1-2 bridge:
log4j.appender.file=org.apache.log4j.DailyRollingFileAppender
log4j.appender.file.File=logs/kurs.log

anyhow: the java security manager / policy files are not often used and are apparently on the way out, no need to fix - hopefully the one who has the same issue finds this comment :-)

@ppkarwasz
Copy link
Copy Markdown
Contributor

@merkisoft,

The main point of @stefanvodita's PR is that File.mkdirs returns false if the directory exits or a file system error occurs, we can not differentiate the two cases. Files.createDirectories on the other hand throws an exception only in the latter case.

I don't see anything wrong in adding a Files.exists call in front of Files.createDirectories. You can create a PR for it.

Remark: with Files.exists you still need to add a "read" permission on the log dir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants