Improve CLI error handling a bit#6844
Conversation
|
(execution 26320237296 / attempt 1) |
📝 WalkthroughWalkthroughThis PR replaces exit/assert failure paths with explicit error results: numeric parsing (str_to_size_t) returns std::optional, NumaConfig::from_string returns std::optional, UCI spin parsing validates via errno-aware checks, Position::flip returns std::optional, perft helpers and Engine::perft return std::variant<uint64_t, PositionSetError>, and UCI integration tracks currentCmd and terminates on reported errors. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b44d065c-c4ce-486c-b84a-c27aee12ac4f
📒 Files selected for processing (11)
src/engine.cppsrc/engine.hsrc/misc.cppsrc/misc.hsrc/numa.hsrc/perft.hsrc/position.cppsrc/position.hsrc/uci.cppsrc/uci.hsrc/ucioption.cpp
| for (size_t c = *cfirst; c <= *clast; ++c) | ||
| { | ||
| indices.emplace_back(c); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section around the reported lines
sed -n '1030,1105p' src/numa.h
# Find the function that owns this loop and related parsing helpers
rg -n "indices_from_shortened_string|from_string|str_to_size_t|cfirst|clast" src/numa.h
# Print the full implementations for the functions we found (to understand reachability of SIZE_MAX)
python3 - <<'PY'
import re, subprocess, os, textwrap, json, sys
path="src/numa.h"
# Extract approximate function blocks by printing from match line to next '}'; heuristic via braces depth.
text=open(path,'r',encoding='utf-8').read().splitlines()
keywords=["indices_from_shortened_string","from_string","str_to_size_t"]
for kw in keywords:
for i,line in enumerate(text, start=1):
if kw in line:
start=i
break
else:
continue
# naive brace tracking from first '{' after start
s=start
# find first '{' after s
depth=0
started=False
out=[]
for j in range(s-1,len(text)):
out.append((j+1,text[j]))
if not started:
if '{' in text[j]:
started=True
depth += text[j].count('{') - text[j].count('}')
else:
depth += text[j].count('{') - text[j].count('}')
if depth<=0:
break
print("\n===== Extracted around", kw, "starting at line", start, "=====")
for ln,content in out[:220]:
print(f"{ln}:{content}")
PYRepository: official-stockfish/Stockfish
Length of output: 6223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "str_to_size_t\s*\(" src/numa.h
rg -n "from_string\(" -S src/numa.h
rg -n "NumaConfig::from_string|from_string<|from_string\s*\(" src | head -n 200
# Print str_to_size_t definition (or nearest) in src/numa.h
# We'll search for "str_to_size_t" without relying on line extraction.
python3 - <<'PY'
import subprocess, re, os, textwrap, sys
path="src/numa.h"
lines=open(path,'r',encoding='utf-8').read().splitlines()
for i,l in enumerate(lines,1):
if "str_to_size_t" in l:
if "static" in l or "template" in l or "auto" in l or "std::optional" in l:
pass
# Find function signature
for i,l in enumerate(lines,1):
if re.search(r"\bstr_to_size_t\b", l) and ('static' in l or 'inline' in l or 'auto' in l):
start=i-20
end=i+80
start=max(1,start); end=min(len(lines),end)
print(f"\n--- context around line {i} ({path}) ---")
for ln in range(start,end+1):
print(f"{ln}:{lines[ln-1]}")
break
else:
print("No obvious definition found in src/numa.h; printing all occurrences with line numbers only.")
for i,l in enumerate(lines,1):
if "str_to_size_t" in l:
print(i, l.strip())
PYRepository: official-stockfish/Stockfish
Length of output: 4761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate str_to_size_t definition/guards in numa.h
rg -n "str_to_size_t" src/numa.h
# Find how NumaConfig::from_string is used (likely CLI path)
rg -n "NumaConfig::from_string|from_string\s*\(" src | head -n 200
# Print a larger contextual window around str_to_size_t (in case it's defined elsewhere in the file)
python3 - <<'PY'
import re
path="src/numa.h"
lines=open(path,'r',encoding='utf-8').read().splitlines()
hits=[i for i,l in enumerate(lines,1) if re.search(r"\bstr_to_size_t\b", l)]
print("str_to_size_t occurrences:", hits[:30], ("..." if len(hits)>30 else ""))
# Heuristic: find the first line that looks like a function definition
defline=None
for i,l in enumerate(lines,1):
if re.search(r"\bstr_to_size_t\b", l) and re.search(r"\bstatic\b|\binline\b|auto\s+\w*\s*\(|std::optional", l):
defline=i
break
if defline is None and hits:
defline=hits[0]
if defline:
start=max(1,defline-60); end=min(len(lines),defline+200)
print(f"\n--- Context around str_to_size_t near line {defline} ---")
for ln in range(start,end+1):
print(f"{ln}:{lines[ln-1]}")
PYRepository: official-stockfish/Stockfish
Length of output: 10926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "str_to_size_t" -S .
rg -n "str_to_size_t\s*\(" -S . | head -n 200
# Try to find a definition pattern (function returning optional size_t)
rg -n "str_to_size_t.*optional<.*size_t" -S . || true
rg -n "optional\s*<\s*size_t\s*>\s*str_to_size_t" -S . || true
# Print likely definition area by searching for "str_to_size_t(const" or "str_to_size_t(std::" etc.
rg -n "str_to_size_t\s*\(const std::string" -S . || true
rg -n "static\s+.*str_to_size_t" -S . || trueRepository: official-stockfish/Stockfish
Length of output: 1198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where str_to_size_t is implemented/validated (to see if SIZE_MAX is rejected)
rg -n "str_to_size_t" -S .
rg -n "str_to_size_t\\s*\\(" -S . | head -n 200
rg -n "optional\\s*<\\s*size_t\\s*>\\s*str_to_size_t|str_to_size_t.*optional<.*size_t|str_to_size_t\\s*\\(.*std::string" -S .Repository: official-stockfish/Stockfish
Length of output: 1296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '450,540p' src/misc.cpp
sed -n '80,140p' src/misc.h
sed -n '200,270p' src/engine.cppRepository: official-stockfish/Stockfish
Length of output: 6062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_numa_config_from_option" -S src
rg -n "NumaConfig" src/engine.cpp src/*.cpp src/*.h | head -n 200
# Find UCI option name or parsing of "Numa" options
rg -n "numa.*option|option.*numa|Numa.*Config|set_numa" -S src | head -n 200Repository: official-stockfish/Stockfish
Length of output: 8930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_numa_config_from_option" -S src
rg -n "NumaConfig" src/engine.cpp src/*.cpp src/*.h | head -n 200
# Find UCI option name or parsing of "Numa" options
rg -n "numa.*option|option.*numa|Numa.*Config|set_numa" -S src | head -n 200Repository: official-stockfish/Stockfish
Length of output: 8930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "NumaPolicy|NumaConfig|numa.*Policy|set_numa_config_from_option|Numa.*Config" -S src
rg -n "setoption name .*Numa|Numa.*value|UCI.*Numa|option.*Numa" -S srcRepository: official-stockfish/Stockfish
Length of output: 5369
🏁 Script executed:
sed -n '40,110p' src/engine.cpp
sed -n '200,260p' src/engine.cpp
sed -n '650,720p' src/numa.hRepository: official-stockfish/Stockfish
Length of output: 6122
Fix non-terminating CPU range expansion for NumaPolicy when clast == SIZE_MAX.
NumaConfig::from_string() (driven by the UCI option NumaPolicy in src/engine.cpp) uses indices_from_shortened_string(), which expands "first-last" with for (size_t c = *cfirst; c <= *clast; ++c). Since str_to_size_t() accepts SIZE_MAX, the size_t increment can overflow and wrap to 0, making c <= *clast stay true forever.
Proposed fix
- for (size_t c = *cfirst; c <= *clast; ++c)
- {
- indices.emplace_back(c);
- }
+ if (*cfirst <= *clast) {
+ for (size_t c = *cfirst;; ++c) {
+ indices.emplace_back(c);
+ if (c == *clast)
+ break;
+ }
+ }There was a problem hiding this comment.
I mean, you can crash with OOM in other exciting ways as well (just set the values rly big)
probably better is to cap the delta
There was a problem hiding this comment.
@anematode Fair point — capping the delta is the more robust fix since it handles both the wraparound edge case and the OOM-via-huge-range vector in one shot.
Something like this would cover both:
else if (parts.size() == 2)
{
auto cfirst = str_to_size_t(std::string(parts[0]));
auto clast = str_to_size_t(std::string(parts[1]));
if (cfirst.has_value() && clast.has_value()
&& *cfirst <= *clast
&& (*clast - *cfirst) < SYSTEM_THREADS_NB) // or a fixed cap like 65536
{
for (size_t c = *cfirst; c <= *clast; ++c)
indices.emplace_back(c);
}
}Using SYSTEM_THREADS_NB as the cap ties the limit to actual hardware, though a fixed constant (e.g. 65536) may be more predictable if someone explicitly passes a custom config on a large machine. Either way, an invalid/absurdly-wide range silently produces no indices and from_string returns std::nullopt (since n == 0 if all ranges are skipped), which is the right behavior.
Would you like to open a follow-up issue to track this, or address it in this PR?
🧠 Learnings used
Learnt from: robertnurnberg
Repo: official-stockfish/Stockfish PR: 6843
File: src/syzygy/tbprobe.cpp:1766-1769
Timestamp: 2026-05-22T15:45:09.442Z
Learning: In the official-stockfish/Stockfish repository, avoid raising code-review comments about clang-format/code formatting issues. Clang-Format is enforced by dedicated CI, so formatting-related violations should be handled by CI rather than human review comments. In code reviews, focus comments on blocking correctness, security, or functional issues.
|
(execution 26320379951 / attempt 1) |
|
|
||
| set(f, is_chess960(), st); | ||
|
|
||
| assert(pos_is_ok()); |
There was a problem hiding this comment.
uh did you intend to remove an assert?
There was a problem hiding this comment.
set already asserts this
| return PositionSetError("Invalid FEN. Board state encoding ended but cursor not at end."); | ||
|
|
||
| if (pieces(PAWN) & (RANK_1 | RANK_8)) | ||
| if (pieces(PAWN) & (Rank1BB | Rank8BB)) |
There was a problem hiding this comment.
this was an actual mistake in the original code. It seems like we don't actually crash with pawns on the 1st or 8th rank which is probably why we didn't catch it, but obviously we should have this be corrected
|
(execution 26324193120 / attempt 1) |
|
|
||
| bool Option::operator!=(const char* s) const { return !(*this == s); } | ||
|
|
||
| static bool value_in_range(const std::string& v, int min, int max) { |
There was a problem hiding this comment.
i'm still thinking about this, mentioned this on the other issue as well, would it make more sense to just clamp the value but allow it ?
that would make future larger settings possible, e.g. for hash sizes beyond what's currently supported. it also means users could specify larger values, and older versions would simply fall back to the largest supported value available at the time. cc @vondele
There was a problem hiding this comment.
not so sure, by defining the behavior we're essentially extending the standard.
Btw, that's probably also something we should catch and not silently fall back to startpos:
$ ./stockfish
Stockfish dev-20260519-133731f3 by the Stockfish developers (see AUTHORS file)
position 5r2/8/8/8/8/8/4Q3/K1k5 w - - 0 1
d
There was a problem hiding this comment.
maybe a bit of a general problem, that we currently have no diagnostics output about invalid option values.. and do nothing
|
can you rebase on master so CI passes? |
|
(execution 26493854483 / attempt 1) |
|
(execution 26493880992 / attempt 1) |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb5cf0f9-2306-43ec-bcbe-cedbd052d8a5
📒 Files selected for processing (11)
src/engine.cppsrc/engine.hsrc/misc.cppsrc/misc.hsrc/numa.hsrc/perft.hsrc/position.cppsrc/position.hsrc/uci.cppsrc/uci.hsrc/ucioption.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/position.h
- src/perft.h
- src/engine.h
- src/numa.h
- src/ucioption.cpp
- src/position.cpp
- src/engine.cpp
- src/uci.h
- src/uci.cpp
There was a problem hiding this comment.
Pull request overview
This PR tightens command-line/UCI parsing and option handling to reduce undefined behavior and uncaught exceptions from invalid input, extending recent FEN validation work. It also changes some previously “hard-fail” paths (e.g., NUMA config parsing, perft position setup) to return structured errors that can be reported or handled.
Changes:
- Replace exception-prone numeric parsing (
stoi/stoull) with checkedstrtoll/strtoull-based helpers returning validity/std::optional. - Convert several “exit/assert/void” APIs to return
std::optional/std::varianterror results (flip/perft/NUMA parsing), and propagate them to UCI. - Improve case-insensitive string handling by avoiding
std::tolowerUB on negativechar.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ucioption.cpp | Adds a range-checking numeric parser for spin options; fixes tolower UB in comparator. |
| src/uci.h | Adjusts UCIEngine APIs/state for improved error reporting (stores current command). |
| src/uci.cpp | Propagates errors from flip/perft/position; adds parsing failure handling and command context tracking. |
| src/position.h | Changes Position::flip() to return an optional error instead of void. |
| src/position.cpp | Returns errors from flip() via set(); minor FEN validation constant tweak. |
| src/perft.h | Changes perft to return std::variant to report FEN setup errors. |
| src/numa.h | Makes NUMA parsing return std::optional and updates CPU index parsing to be non-terminating on bad input. |
| src/misc.h | Changes str_to_size_t to return std::optional<size_t>. |
| src/misc.cpp | Implements non-throwing str_to_size_t using strtoull/errno checks. |
| src/engine.h | Updates Engine APIs to return error-aware results (variant/optional) and boolean NUMA setter outcome. |
| src/engine.cpp | Propagates NUMA parse failure as a non-fatal option update message; perft now returns variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
(execution 26524714796 / attempt 1) |
Reduce the number of easy ways to hit UB or an uncaught exception. This builds on Sopel's work on FEN validation.
Obviously SF is not and will never be a security boundary, but at least we can reduce the number of reports about uncaught
std::stoiout of range, etc.I also added a gentle fallback to
NumaConfigspecifically because that's something that (as a dev) I frequently set by hand in the CLI, but happy to revert if that's too annoying of a change.