Segfault on multiple root category definitions introduced in log4shib 1.0.6
Basics
Technical
Logistics
Basics
Technical
Logistics
Description
The fix for introduced a change in log4shib, active as of version 1.0.6, to let the root category take ownership of its Appender pointer. Unfortunately this causes a segfault due to a use-after-free on the Appender* of the root category if the configuration file contains definitions for multiple categories that map to the root appender.
As part of PropertyConfiguratorImpl::configureCategory(), each category is first asked to release all of its existing appenders, and then reassigns them. When asked of the root category, this now causes it to delete its Appender* because it now has ownership of the pointer (per the change).
When configureCategory() gets run twice on the root category, on the second run through the root category will delete its Appender* and then get it reassigned again. At runtime this causes a segfault when trying to access the root category's appender, because the pointer has been freed on the second run through configureCategory().
Because the empty category name also maps to the root category, the issue can be reproduced in two ways:
and
Sample program to reproduce: {{
Environment
None
Activity
Show:
Scott Cantor April 17, 2018 at 1:40 AM
Looks fine to me, albeit this isn't really something I'm all that concerned about, this is pretty deliberate local "intentional" breakage so nothing to be all that worried about, just better input checking.
Rod Widdowson February 25, 2017 at 3:39 PM
fd0bb18 fixes the problem as described by the OP.
It certainly stops the crash from happening, but I do not grock log4shib so the change is made in ignorance.
I'll ask Scott to eyeball it.
Rod Widdowson February 25, 2017 at 3:00 PM
Reproduced thanks to excellent example.
Still thinking about this. I cannot see that we can change PropertyConfiguratorImpl::getCategories output to a std::set without an major version bump.
I guess I could use a set to police the vector.
Unidentified Legacy Account July 10, 2016 at 8:37 PM
Edited
I hit Enter too soon >_>
Sample program:
Because there are 2 ways to hit this, there are 2 fixes that be should be put in place. Here are the ones I've found to work:
Make PropertyConfiguratorImpl::getCategories output to a std::set instead of a std::list, so that rootCategory cannot be hit multiple times.
Modify PropertyConfigurationImpl::getCategories to reject categories with an empty name, as they will also resolve to the root category (cfr. Category::getRoot)
Note that upstream log4cpp is not affected because it does not have the ownership modification of the root category.
Fixed
Pinned fields
Click on the next to a field label to start pinning.
The fix for introduced a change in log4shib, active as of version 1.0.6, to let the root category take ownership of its Appender pointer. Unfortunately this causes a segfault due to a use-after-free on the Appender* of the root category if the configuration file contains definitions for multiple categories that map to the root appender.
As part of PropertyConfiguratorImpl::configureCategory(), each category is first asked to release all of its existing appenders, and then reassigns them. When asked of the root category, this now causes it to delete its Appender* because it now has ownership of the pointer (per the change).
When configureCategory() gets run twice on the root category, on the second run through the root category will delete its Appender* and then get it reassigned again. At runtime this causes a segfault when trying to access the root category's appender, because the pointer has been freed on the second run through configureCategory().
Because the empty category name also maps to the root category, the issue can be reproduced in two ways:
and
Sample program to reproduce:
{{