Conversation
GeoLookup is a process wide singleton and GeoLookup::setDataBase() used to call MMDB_open() unconditionally, overwriting the MMDB_s member without closing the database that was open before. Every additional evaluation of SecGeoLookupDb therefore leaked the MMDB_s heap metadata plus the mmap()ed copy of the whole database file. This is not limited to a configuration that repeats the directive: under nginx the master process re-parses all rules on every reload and on every configuration test, and each server/location context that includes the directive parses it again, so the leak accumulates for the lifetime of the master process and is inherited by every worker. cleanUp() already knows how to release either back end and resets m_version, so calling it at the top of setDataBase() is enough. The GeoIP back end was not leaking (GeoIP_open is guarded by m_version == NOT_LOADED), but it kept the first database forever and ignored any later SecGeoLookupDb; it is now replaced like the MaxMind one. Behaviour change on the error path: when a SecGeoLookupDb fails to open after a good database was already loaded, the previous database is now released and m_version is left as NOT_LOADED, so a later @geoLookup logs "Database is not open. Use: SecGeoLookupDb directive." Before this change m_version stayed as VERSION_MAXMIND while MMDB_open() had already clobbered the MMDB_s holding the good database, so @geoLookup set no GEO variable and reported the confusing "MaxMind: Got an error from libmaxminddb: The MaxMind DB file's search tree is corrupt" instead. In both cases the configuration carrying the failing directive is rejected with a parser error. Out of scope, and deliberately not changed: ~ModSecurity only calls GeoLookup::cleanUp() under WITH_GEOIP. Calling it for MaxMind as well would be wrong under nginx, where the old cycle's ModSecurity instance is destroyed *after* the new cycle has already parsed its rules -- it would close the database the new configuration is about to use. The same hazard already exists for the GEOIP build.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
ChangesGeo lookup database reload handling
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
GeoLookup::setDataBase()calls the existingcleanUp()before opening the new database, so a previously loaded database is released instead of overwritten.test/test-cases/regression/config-secgeolookupdb.json:SecGeoLookupDblisted twice with the same file, plus a@geoLookuprule that must still resolveGEO:COUNTRY_NAME.why
GeoLookupis a process-wide singleton. UnderWITH_MAXMIND,setDataBase()calledMMDB_open()into themmdbmember unconditionally, so the previous handle (heap metadata plus the mmap of the whole database file) was never closed.cleanUp()was only reached from the singleton destructor and, for GeoIP builds only, from~ModSecurity.SecGeoLookupDbtherefore leaked one database mapping: everynginx -s reload, everynginx -t, and every server or location whose rules include the directive within one configuration. For GeoLite2-City that is roughly 70 MB of address space per parse plus one more mapping towardvm.max_map_count, accumulating in the master and inherited by workers.SecGeoLookupDbthat fails after a good one now leaves the singleton inNOT_LOADED(lookups log "Database is not open" instead of "search tree is corrupt" over a clobbered struct); in GeoIP-only builds a later directive now replaces the database instead of being silently ignored.~ModSecuritystill callscleanUp()only underWITH_GEOIP. Adding it for MaxMind would close the database the new nginx cycle is already using, since the old cycle's instance is destroyed after the new configuration is parsed. That ordering hazard already exists for GeoIP builds and is out of scope here.Evidence, unfixed tree,
valgrind --leak-check=full ./.libs/regression_tests test-cases/regression/config-secgeolookupdb.json(MaxMind-only build):The mmap of the database file itself is not tracked by leak-check, so the real loss is larger than the bytes shown. With the fix:
All heap blocks were freed -- no leaks are possible;variable-GEO.jsonstill passes.references
Summary by CodeRabbit
Bug Fixes
Tests