rate_limit: YAML parse errors escape yamlParser() and terminate ATS on config reload
- 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
Assessment
This issue has not been assessed yet.