Don't crash parsing CLIENT INFO with '=' or spaces in a field value - #4275
Don't crash parsing CLIENT INFO with '=' or spaces in a field value#4275eeshsaxena wants to merge 3 commits into
Conversation
parse_client_info split each token on '=' with no maxsplit and tokenized on arbitrary whitespace, so a field value containing '=' (a client name like "foo=bar", which Redis allows) or a space (a unix-socket addr such as "/tmp/redis sock/redis.sock") raised ValueError. This also broke CLIENT INFO responses and ACL LOG parsing, which embeds client-info. Reuse the tokenization parse_client_list already applies: split on '=' once and reattach space-separated continuations. Adds a regression test.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit f50a481. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f50a4812de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Values may include spaces. For instance, when running Redis via a | ||
| # Unix socket, the addr/laddr field can be a path such as | ||
| # "/tmp/redis sock/redis.sock"; reattach the split-off remainder. | ||
| client_info[last_key] += " " + token |
There was a problem hiding this comment.
When value is empty or whitespace-only, strip().split(" ") yields [''], so the first token reaches this branch with last_key is None and raises KeyError; the previous split() loop returned {}. This also affects the legacy ACL LOG parsers, which explicitly pass log_data.get("client-info", ""), so an entry without that optional field now crashes instead of being parsed.
Useful? React with 👍 / 👎.
| if "=" in token: | ||
| # Values might contain '=' (e.g. a client name set to "foo=bar"). | ||
| key, val = token.split("=", 1) | ||
| client_info[key] = val |
There was a problem hiding this comment.
When one value contains both a space and an equals sign—as a valid Unix-socket path such as addr=/tmp/redis sock=prod/redis.sock:0 can—this condition treats the continuation sock=prod/redis.sock:0 as a new field, truncating addr and creating a spurious sock entry. Detect actual client-info field boundaries rather than treating every post-space token containing = as a key.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
An empty or whitespace-only value tokenizes to [''], which hit the space-continuation branch with last_key still None and raised KeyError. ACL LOG entries omit client-info as '' (parse_acl_log), so guard the continuation on last_key. Adds coverage.
|
Good catch on the empty-input case, that was a real regression. Fixed it so an empty or whitespace-only client-info (like an ACL LOG entry missing that field) stays {} instead of raising, with a test added. The space-plus-equals value is an inherent ambiguity that parse_client_list resolves the same way, so I kept the two consistent rather than special-casing it. |
Mukller
left a comment
Mukller
left a comment
There was a problem hiding this comment.
Verified locally against the PR branch (installed redis 8.1.0 venv, ran from repo checkout so the patched module shadows site-packages):
Tests: pytest tests/test_parsers/test_helpers.py — 22/22 green, including the two new cases.
Adversarial matrix through the new parse_client_info:
| input | result |
|---|---|
id=7 addr=/tmp/redis sock/redis.sock:0 |
addr = /tmp/redis sock/redis.sock:0 ✓ (the unix-socket case) |
name=foo=bar=baz age=42 |
name = foo=bar=baz, age coerced to int ✓ |
lib-ver= (empty value) |
stored as "" ✓ |
totally-broken-token id=3 |
junk token skipped instead of ValueError (old code crashed) |
The root fix is right: split("=", 1) + reattachment via last_key, with the last_key is None sentinel correctly keeping empty ACL-LOG client-info as {}.
One minor nit, non-blocking: because .split(" ") no longer collapses whitespace like the old .split(), a double space between two pairs attaches an empty token to the previous value:
parse_client_info("x=1 y=2")
# -> {'x': '1 ', 'y': '2'} # note trailing space on 'x'Real CLIENT INFO output separates pairs with single spaces, so this shouldn't occur in practice — but if you want strict parity with the old parser's whitespace collapsing, you could skip empty tokens in the reattachment branch (if not token: continue). Either way fine by me.
CI question: the failing job is the single combo "Redis 8.4.3; Python 3.14; plain parser; asyncio; 2-unified_responses". Under RESP3/unified responses, does CLIENT INFO still reach parse_client_info, or does it arrive pre-parsed as a map (making this failure unrelated)? If unrelated/flaky, a re-run would clean it up.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @eeshsaxena, thank you for your contribution!
The defect is real and worth fixing: parse_client_info unpacks token.split("=") into exactly two values, so it raises ValueError both for a value containing = (Redis allows = in a client name) and for a space in a unix-socket addr/laddr. It reaches CLIENT INFO and all three ACL LOG parsers, on both stacks and every protocol/topology combination. #3797 fixed exactly this in parse_client_list, and this function was simply left behind - so aligning the two is the right call, and the empty-input guard you added in the second commit is a good catch.
One code change before merge: parse_client_list and parse_client_info now hold identical tokenization that has already diverged - only the new copy guards last_key is not None, while the older one would raise KeyError on that path. Please fold the loop into a single module-private helper in redis/_parsers/helpers.py and call it from both, so the guard applies to CLIENT LIST too and the two parsers of the same server-side blob cannot drift again.
Additionally: an integration test that does client_setname("test=name") and asserts the name round-trips through client_info() would cover the real server path alongside the unit tests.
Please also check the bot's comments.
petyaslavova
added
maintenance
parse_client_list and parse_client_info parsed the same server blob with copy-pasted loops that had already drifted: only parse_client_info guarded the leading/empty token, so parse_client_list still raised KeyError on that path. Extract one _parse_client_info_fields helper and call it from both, which also collapses a stray double space between pairs. Adds a unit test for the CLIENT LIST guard and an integration test that a client name with '=' round-trips through client_info() on both stacks.
|
Thanks for the review. Both points addressed:
Also added a unit test that |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c52bbb110
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not token: | ||
| # Collapse runs of whitespace so a double space between pairs does | ||
| # not leave a trailing space glued onto the previous value. | ||
| continue |
There was a problem hiding this comment.
On a Unix-socket deployment whose socket filename contains two or more consecutive spaces, discarding empty split components collapses those spaces in addr/laddr. In particular, CLIENT LIST previously reattached each empty component and preserved the exact address, whereas this shared parser silently turns /tmp/redis sock/redis.sock:0 into /tmp/redis sock/redis.sock:0; retain empty components when they belong to a continued field value rather than treating every run as redundant separators.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround - the shared _parse_client_info_fields helper and the sync/async round-trip tests are exactly what was needed, and CI is fully green now, including the RESP3 unified-responses job that was failing earlier.
One change left. The if not token: continue branch collapses runs of spaces, which silently regresses CLIENT LIST for the case #3797 fixed: on master, addr=/tmp/redis␣␣sock/r.sock:0 round-trips verbatim, but on this branch it comes back as /tmp/redis␣sock/r.sock:0. A file name can contain consecutive spaces, so that path is reachable, whereas the double-space-between-pairs nit it was added for cannot occur in real server output. Please drop the continue and rely only on the last_key is not None guard - the empty-input fix still holds, since the single empty token from "" or " " hits that guard and yields {}, and CLIENT LIST stays byte-identical to today. The second assertion in test_parse_client_list_shares_guard will need updating; an assertion that a double-space socket path survives both parsers unchanged would be a better fit.
On the remaining bot comment about a value holding both a space and an = after it: no action needed. That ambiguity is unresolvable without pinning a closed set of field names, which would break whenever the server adds one. Keeping both parsers consistent is the right call.
3 participants
parse_client_info splits each field token on '=' with no maxsplit and tokenizes on arbitrary whitespace, so a value containing '=' (a client name like "foo=bar", which Redis allows) or a space (a unix-socket addr like "/tmp/redis sock/redis.sock") raises ValueError. This trips up both CLIENT INFO responses and ACL LOG parsing, which embeds the same client-info.
The sibling parse_client_list already handles both cases, so this reuses that same tokenization (split on '=' once, reattach space-separated continuations) and adds a regression test.
Note
Low Risk
Localized response-parser change with regression tests; no auth, networking, or API contract changes beyond fixing previously broken edge-case values.
Overview
Fixes crashes and wrong field splits when Redis returns CLIENT INFO, CLIENT LIST, or ACL LOG
client-infoblobs where values contain=(e.g. client nametest=name) or spaces (e.g. Unix-socketaddrpaths).Introduces shared
_parse_client_info_fields(split only on the first=, reattach space-separated continuation tokens, safe empty/malformed input) and wires bothparse_client_infoandparse_client_listthrough it so ACL LOG and list parsing stay consistent. Adds unit tests for the tokenizer edge cases and integration tests that round-tripclient_setname("test=name")viaclient_info()for sync and asyncio clients.Reviewed by Cursor Bugbot for commit 0c52bbb. Bugbot is set up for automated code reviews on this repo. Configure here.