Closed
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a hard cap on the in-memory HSTS cache size to prevent unbounded growth, and adds a new unit/integration test to verify the cap behavior when loading from a file.
Changes:
- Add
MAX_HSTS_ENTRIES(1,000) and enforce it by evicting the oldest entry on insert. - Add unit test
unit1674and test definitiontest1674to validate the cache is capped at 1,000 entries when loading more. - Register the new test in the unit and data makefiles.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/hsts.h |
Introduces MAX_HSTS_ENTRIES to define the HSTS cache cap. |
lib/hsts.c |
Adds capped append helper that evicts the oldest entry before appending when at limit. |
tests/unit/unit1674.c |
New unit test that renders an over-limit HSTS file, loads it, and prints resulting entry count. |
tests/unit/Makefile.inc |
Registers unit1674.c in the unit test build list. |
tests/data/test1674 |
Adds a runtests.pl test case that validates stdout reports 1,000 entries. |
tests/data/Makefile.am |
Registers test1674 in the test data list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Avoid never-ending growth. When adding more entries, it now deletes the first entry in the list, which is the oldest added entry still held in memory. I decided to avoid a Least Recently Used concept as I suspect with a list with this many entries most entries have not been used, and we don't save the timestamp of recent use anyway. The net effect might (no matter what) be that the removed entry might feel a bit "random" in the eyes of the user. Verify with test 1674 Ref #21183
Member
Author
|
augment review |
🤖 Augment PR SummarySummary: Prevent unbounded growth of the in-memory HSTS cache by enforcing a hard cap. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
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
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.
Avoid never-ending growth.
When adding more entries, it now deletes the first entry in the list, which is the oldest added entry still held in memory. I decided to avoid a Least Recently Used concept as I suspect with a list with this many entries most entries have not been used, and we don't save the timestamp of recent use anyway.
The net effect might (no matter what) be that the removed entry might feel a bit "random" in the eyes of the user.
Verify with test 1674
Ref #21183