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

Hardens PropertiesUtil against recursive property sources #3263

Merged
merged 5 commits into from
Dec 9, 2024

Conversation

ppkarwasz
Copy link
Contributor

As showed in #3252, Spring's JndiPropertySource not only can throw exceptions, but can also perform logging calls. Such a call causes a recursive call to PropertiesUtil.getProperty("log4j2.flowMessageFactory") and a StackOverflowException in the best scenario. The worst scenario includes a deadlock.

This PR:

  • Moves the creation of the default MessageFactory and FlowMessageFactory to the static initializer of LoggerContext. This should be close enough to the pre-2.23.0 location in AbstractLogger. The LoggerContext class is usually initialized, before Spring Boot adds its property sources to PropertiesUtil.
  • Adds a check to PropertiesUtil to ignore recursive calls.

Closes #3252.

As showed in #3252, Spring's `JndiPropertySource` not only can throw exceptions, but can also perform logging calls.
Such a call causes a recursive call to `PropertiesUtil.getProperty("log4j2.flowMessageFactory"`) and a `StackOverflowException` in the best scenario. The worst scenario includes a deadlock.

This PR:

- Moves the creation of the default `MessageFactory` and `FlowMessageFactory` to the static initializer of `LoggerContext`. This should be close enough to the pre-2.23.0 location in `AbstractLogger`. The `LoggerContext` class is usually initialized, before Spring Boot adds its property sources to `PropertiesUtil`.
- Adds a check to `PropertiesUtil` to ignore recursive calls.

Closes #3252.
@ppkarwasz ppkarwasz self-assigned this Dec 4, 2024
@ppkarwasz ppkarwasz requested a review from vy December 6, 2024 16:55
The constructor is effectively package-private, since it has a package-private class (`AsyncLoggerDisruptor`) in its signature.
@ppkarwasz ppkarwasz merged commit 18a1deb into 2.24.x Dec 9, 2024
7 checks passed
@ppkarwasz ppkarwasz deleted the fix/2.24.x/3252_recursive_property_source branch December 9, 2024 08:01
@sundaybluesky
Copy link

sundaybluesky commented Mar 5, 2025

Hi @ppkarwasz,

Can you please consider change back AsyncLogger constructor package-private access back to public? I've extended AsyncLogger class with the same package name to build a customized AsyncLogger and use reflection to initial customized AsyncLogger. This code was working for more than 7 years and blocked in 2.24.3 with this PR code change.

Thanks,
sundaybluesky

@ppkarwasz
Copy link
Contributor Author

Hi @sundaybluesky,

Can you please consider change back AsyncLogger constructor package-private access back to public?

Sure, please open a new feature request. In the new feature request, can you explain how are you using AsyncLogger. It is entirely possible that there are already public APIs that allow you to do what you are doing. For example AsyncLoggerContext.newLogger() allows you to create a new AsyncLogger without any reflection.

This code was working for more than 7 years and blocked in 2.24.3 with this PR code change.

That is the problem with using reflection to access private methods or classes like AsyncLoggerDisruptor: the class or method can disappear without notice.

Note: As far as I understand you called the constructor of AsyncLogger through reflection. You can still do it, but you need to make it accessible first.

@sundaybluesky
Copy link

sundaybluesky commented Mar 11, 2025

Hi @ppkarwasz ,

Thanks for you quick response. I created #3527 to track this issue, please help to take a look.

Regards,
sundaybluesky

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