[FIXED JENKINS-44330] - Prevent classloading of Target comparator in LogRecorder#orderedTargets() #2894
Conversation
…LogRecorder#orderedTargets() It is just a hotfix, there may be other LogRecorders affected. Ideally we need a response from Jetty maintainers to jetty/jetty.project#1563. No tests since I see no way to trigger such classloading + no actual need in it.
oleg-nenashev
added
needs-more-reviews
|
Bonus: it improves performance a bit. |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| public int compare(Target left, Target right) { | ||
| return right.getName().length() - left.getName().length(); | ||
| } | ||
| } |
There was a problem hiding this comment.
@oleg-nenashev how this change can prevent a StackOverflow?
There was a problem hiding this comment.
The class will be loaded in Static initializer instead of the orderedTargets() call. Static initilizer will be run when the class is first accessed, hence it will happen long before the Recorder starts receiving the data
There was a problem hiding this comment.
@oleg-nenashev Thanks. It looks like a bad design of LogRecorder.
There was a problem hiding this comment.
Well logging systems generally are sensitive to this class of bugs: you have to be very careful to not run any potentially novel classes in the course of handling a log event.
| public int compare(Target left, Target right) { | ||
| return right.getName().length() - left.getName().length(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Well logging systems generally are sensitive to this class of bugs: you have to be very careful to not run any potentially novel classes in the course of handling a log event.
|
@reviewbybees done. Will merge towards the next weekly if nobody votes against |
oleg-nenashev
added
ready-for-merge
It is just a hotfix, there may be other LogRecorders affected. Ideally we need a response from Jetty maintainers to jetty/jetty.project#1563. No tests since I see no way to trigger such classloading + no actual need in it.
See JENKINS-44330.
Changelog entries
Proposed changelog entries:
Submitter checklist
Desired reviewers
@reviewbybees @olamy @damphyr