apache / apache/trafficserver

rate_limit: YAML parse errors escape yamlParser() and terminate ATS on config reload

Open
#13,598 0 comments 0 reactions 1 assignee Claimed by @sinhaparth5 View on GitHub
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

`SniSelector::yamlParser()` wraps only `YAML::LoadFile` in a try block
(`sni_selector.cc:35-43`). Every node accessor and `.as()` call after it runs
unguarded, so a malformed value in the rate_limit config throws a
`YAML::Exception` that escapes the function.

On startup that replaces the intended `TSFatal` with an unhandled exception. On
`traffic_ctl config reload` it kills a running server: `sni_config_cont` is the
callback registered with `TSMgmtUpdateRegister`, so the throw at
`sni_selector.cc:191` unwinds into the event loop. The `else` branch directly
below that call already handles a failed reload by logging and keeping the old
config, and it never gets the chance to run.

### Reproduce

Either config throws. Load it, then run `traffic_ctl config reload`.

A selector entry with no `sni` key:

```yaml
selector:
- limit: 100
```

`sni["sni"].IsSequence()` at `sni_selector.cc:116` throws `YAML::InvalidNode`
on the const node, before the "selector node is not a map or without a name"
error below it can fire.

A `percentage` value taken from the documentation:

```yaml
ip-rep:
- name: test
size: 15
percentage: 0.9
```

`ip_reputation.cc:93` reads it as `uint32_t`, so `0.9` throws
`YAML::TypedBadConversion`.

Compiled against the vendored yaml-cpp, the two cases report:

```
invalid node; first invalid key: "sni"
yaml-cpp: error at line 1, column 13: bad conversion
```

The same exposure applies to the other conversions: `limit`, `rate`,
`queue.size`, `queue.max_age`, `metrics.prefix`, `metrics.tag` (`limiter.h`),
`buckets`, `size`, `max_age`, `perma-block.*` (`ip_reputation.cc`), `cidr`
(`lists.cc`), and `ip-rep` / `exclude` (`sni_limiter.cc`).

### On `percentage`

`doc/admin-guide/plugins/rate_limit.en.rst:272` gives the default as ``0.9``,
but the parser reads an integer and `_percentage` defaults to `90`
(`ip_reputation.h:249`). The documented value is the one that throws, so the
docs need `90` here.

Present on master (a2011c2fc). The unguarded conversions came in with #10559.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.