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

Fix numeric configuration keys handling #198

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

aleksmaus
Copy link
Member

This is one of the ways to fix the issue with configuration parsing where the keys are numeric strings.

Couple of example of problem when creating configuration with NewFrom API call:

INPUT:

{"a":{"9223372036854775807":{}}}

RESULT:

panic: runtime error: makeslice: len out of range [recovered]
	panic: runtime error: makeslice: len out of range

INPUT:

{"a":{"9223372036854":{}}}

RESULT:

runtime: out of memory: cannot allocate 147573956411392-byte block (3833856 in use)
fatal error: out of memory

Solution

The library is quite old, so being overly cautious here in order to avoid breaking the original behavior. One of the possible approaches here is to preserve the same behavior as before, put some protection from large allocations and OOM, allow the user to enable a different treatment of the index (numeric) configuration fields.

Changes

  1. Implement some sane, configurable (MaxIdx option) limit on the "index" field value, in order to prevent huge memory allocations and crashes. It's set to 1024 by default. The numeric field will be converted to the array as before only if the numberic value is less or equal to the max set. This is the first "line of defense" that is protecting the existing implementation from blowing up the process.

  2. Implement EnableNumKeys option in order to enable the "numeric keys" in the configuration. This allows the numeric keys and preserves it's value as is. This is disabled by default in order to preserve backwards compatibility.

  3. Add unit tests verifying the original problem is fixed and covering the new options.

P.S. Since the library is quite old looks like Go formatting change since then, so got some comments reindented on save.

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. The change is isolated, really like the check for maxIdx, that is a great addition to ensure that the parsing doesn't abuse memory allocation.

Thanks for the tests as well.

@cmacknz cmacknz added the Team:Elastic-Agent Label for the Agent team label Feb 12, 2024
@aleksmaus
Copy link
Member Author

test step seems stuck on this PR "Waiting for status to be reported", and can't merge it says "Required statuses must pass before merging" and no merge button is rendered for me.

@blakerouse blakerouse merged commit db1ecc8 into elastic:main Feb 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants