Bugfix to guard against stack overflow errors caused by very large reg-ex input

[kartg]

Description

This change fixes a code path that did not properly impose the index-level max_regex_length limit. Therefore, it was possibly to provide an arbitrarily large string as the include/exclude reg-ex value under search aggregations. This exposed the underlying node to crashes from a StackOverflowError, due to how the Lucene RegExp class processes strings using stack frames.

Signed-off-by: Kartik Ganesh gkart@amazon.com

Issues Resolved

closes #1992

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dblock]

This is super great work!

[opensearch-ci-bot]

✅   Gradle Check success 23ee87a
Log 4281

Reports 4281

[kartg]

@dblock should this be backported?

[andrross]

When can index settings be null? Seems like this would result in an odd exception message of "...the allowed maximum of [-1]...".

[kartg]

Oops, good catch. This condition should really be:

if (maxRegexLength > 0 && include.length() > maxRegexLength) {

I'll make this change

[kartg]

#2814

[andrross]

The exception text is identical in both cases, right? Maybe an opportunity to have a static helper like validateRegexLength(String regex, IndexSettings settings) or similar

[kartg]

#2814

[dreamer-89]

@kartg : Thanks for pulling up this change and fixing the bug in a short time.

I wanted to share one observation I have around search API response, get feedback and decide if we need to track further. The API returns empty successful result instead of an exception when providing a large string (> max_regex_length) in include on a random field. I think one way to fix it, is to gate the length check in AggregatorFactories where other parsing exceptions are handled (though it would need to index settings reference and is much bigger change).
Or we can return a response suggesting the referenced field is not found.

curl -X POST "localhost:9200/test-index/_search?pretty" -H "Content-Type: application/json" -d '
{
  "query": {
  	"bool":{"filter":[]}
  },
  "aggs":{"suggestions":{"terms":{"field":"random_field", "include":"<large string>","execution_hint":"map","shard_size":10}}}
}

Actual Response: Empty result even though include field exceeds max_regex_length.

{
    "took": 3,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 0,
            "relation": "eq"
        },
        "max_score": null,
        "hits": []
    },
    "aggregations": {
        "suggestions": {
            "doc_count_error_upper_bound": 0,
            "sum_other_doc_count": 0,
            "buckets": []
        }
    }
}

Expected: Should fail before with max_regex_length exception instead of empty response.

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "The length of regex [50001] used in the request has exceeded the allowed maximum of [1000]. This maximum can be set by changing the [index.max_regex_length] index level setting."
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "test-index",
                "node": "w4M35XX3Rtq10akMmt0crg",
                "reason": {
                    "type": "illegal_argument_exception",
                    "reason": "The length of regex [50001] used in the request has exceeded the allowed maximum of [1000]. This maximum can be set by changing the [index.max_regex_length] index level setting."
                }
            }
        ],
        "caused_by": {
            "type": "illegal_argument_exception",
            "reason": "The length of regex [50001] used in the request has exceeded the allowed maximum of [1000]. This maximum can be set by changing the [index.max_regex_length] index level setting.",
            "caused_by": {
                "type": "illegal_argument_exception",
                "reason": "The length of regex [50001] used in the request has exceeded the allowed maximum of [1000]. This maximum can be set by changing the [index.max_regex_length] index level setting."
            }
        }
    },
    "status": 400
}

[kartg]

@dreamer-89 Discussed here - #1992 (comment)

Assuming random_field is not a valid field on test-index, then this code path does not actually parse the input string as a RegExp. Given that this then returns an empty aggregation, this feels like reasonable behavior. Thoughts?

[rishabhmaurya]

Sorry for posting this late, but I'm not convinced with the way this change is implemented due to following reasons -

1. This change adds dependency on IndexSettings for IncludeExclude, which seems logically incorrect and down the line it may interrupt in modularity of codebase for such non-essential dependencies.
2. Also, making IndexSettings to be mandatory, for one of the method, again looks logically incorrect. There could be consumers, who are using this class, which don't have instance of IndexSettings or are not related to index.
3. Changing publicly exposed class constructor/public apis, doesn't seems like a good idea. If its really needed, it should be put on the deprecation path.

[dblock]

@kartg Take a look above, open another issue if you agree

[kartg]

@dblock Tracking in #2858

[jromeroch]

This fix can be backported to version 1.35? We faced this issue, but currently we can't upgrade as we have some blockers.

[dblock]

@jromeroch I think this can qualify, I'll mark it backport 1.x to start, but if auto-backport fails it will need to be done by someone (you?) by hand.

[jameshinsta]

My understanding is that this will be included in 1.3.12 as it is marked for backport 1.x.

Does auto-backport then include all 1.x versions, so we should expect it in 1.3.11?