Repository navigation
feat: integrate toml-c, add server.toml config system (A4 #124) - #173
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a vendored TOML library, a fixed-size TOML-backed server configuration subsystem, integrates config loading into server startup and global state, updates the build to compile new sources, adds unit tests for configuration, and documents the third‑party license. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Server Main
participant ConfigAPI as obc_config_*()
participant TOML as toml_parse/_file
participant CFG as g_server_cfg (in-memory)
participant Runtime as Game Runtime
Main->>ConfigAPI: obc_config_defaults(&g_server_cfg)
Main->>ConfigAPI: obc_config_load(path?, &g_server_cfg)
ConfigAPI->>TOML: toml_parse_file()/toml_parse()
TOML-->>ConfigAPI: toml_table_t* root
ConfigAPI->>ConfigAPI: process_toml(root) (server, game, data, gamespy, master, modules)
ConfigAPI->>CFG: populate fields & arrays
ConfigAPI-->>Main: return success/failure
Main->>Runtime: map g_server_cfg -> runtime globals (port, players, map, masters, logs, etc.)
Runtime->>Runtime: use globals during server lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/config.c`:
- Around line 24-205: Split the large process_toml function into smaller
section-specific static helpers to reduce complexity: create static void
process_server(toml_table_t *root, obc_server_cfg_t *cfg), process_game(...),
process_data(...), process_gamespy(...), process_master(...), and
process_modules(...), move the corresponding parsing blocks from process_toml
into each helper (preserving all field assignments, str_copy/free logic, array
bounds checks, and kv handling for modules), keep the helper functions static
for internal linkage, and replace the inline blocks in process_toml with calls
to these new helpers (ensuring any initialization like cfg->module_count or
cfg->mod_pack_count remains correct and behavior is unchanged).
- Around line 341-351: The function obc_config_mod_float uses strtod(v, NULL)
which returns 0.0 on parse failure, so replace the NULL end-pointer usage with
an endptr and validate conversion: set errno = 0, call strtod(v, &endptr), check
that endptr != v (meaning at least one char was parsed), ensure errno is not
ERANGE (or handle range errors appropriately), and that trailing characters are
acceptable (e.g., only whitespace). If any check fails, return default_val;
otherwise return the parsed double. Use the existing helpers find_module and
find_kv to locate v before parsing.
- Around line 353-363: obc_config_mod_bool currently treats any unrecognized
string as false instead of honoring default_val; update obc_config_mod_bool to
explicitly handle both true-y values ("true","1") returning true and false-y
values ("false","0") returning false, and for any other/unrecognized string
return default_val; locate the function obc_config_mod_bool (and helper calls
find_module and find_kv) and change the final return logic so unrecognized v
falls back to default_val rather than returning false.
- Around line 329-339: obc_config_mod_int currently calls strtol(v, NULL, 10)
and thus treats parse failures (e.g., "abc") as 0; update obc_config_mod_int to
use a char *endptr and call strtol(v, &endptr, 10), check that endptr != v (no
digits parsed) and that errno is not set to ERANGE for overflow/underflow, and
if any of these indicate failure return default_val; keep the existing
short-circuit checks using find_module and find_kv unchanged.
- Around line 159-199: The loop that reads module config keys using
toml_table_len/toml_table_key may iterate non-scalar entries (arrays/tables)
which will fail all scalar probes (toml_table_string/int/bool/double) and
silently skip them; add a concise comment above the loop (or immediately before
the scalar-type probes) explaining that nested tables/arrays in the
[modules.config] sub-table are intentionally unsupported and therefore skipped,
referencing the loop that uses toml_table_key(), toml_table_len(), and the m->kv
storage (obc_mod_kv_t, m->kv_count) so future readers understand this behavior
and it is not an accidental omission.
In `@src/server/main.c`:
- Around line 182-218: The TOML-loaded masters in g_server_cfg.masters are being
appended into user_masters[] (via user_master_count) so later CLI --master
entries end up added instead of replacing them; change the CLI parsing for
--master so that on the very first CLI-provided master you reset
user_master_count to 0 (clearing any TOML entries) before adding the CLI
entries, ensuring user_masters and user_master_count reflect only CLI masters
when any --master is supplied.
In `@src/toml/toml.c`:
- Around line 51-92: The helper functions read_unicode_escape,
toml_table_unparsed, toml_array_unparsed, toml_value_string, toml_value_bool,
toml_value_int, toml_value_double, toml_value_timestamp and is_leap should be
given internal linkage: add the static keyword to their forward declarations
near the top (the typedef and prototypes around
toml_table_unparsed/toml_array_unparsed/toml_value_* and read_unicode_escape)
and also add static to their corresponding definitions later in the file (the
definitions around lines where read_unicode_escape and the toml_value_*/is_leap
implementations appear) so these symbols are file-local and won’t pollute the
global symbol table.
In `@tests/test_config.c`:
- Around line 332-345: The obc_config_mod_bool function currently treats any
value other than "true"/"1" as false which makes unrecognized strings silently
override the default; update obc_config_mod_bool in src/server/config.c to
explicitly accept "true"/"1" as true, explicitly accept "false"/"0" as false,
and in all other cases return the provided default_val; ensure comparisons are
case-insensitive or normalize input if appropriate and reference the
cfg/module/key lookup logic already in obc_config_mod_bool when applying the
fix.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
LICENSE-THIRD-PARTYMakefileinclude/openbc/config.hinclude/openbc/server_state.hserver.tomlsrc/server/config.csrc/server/main.csrc/server/server_state.csrc/toml/LICENSEsrc/toml/toml.csrc/toml/toml.htests/test_config.c
🔴 H1: Integer config values lack range validationFile: Several integer fields are cast from Port (line ~32): v = toml_table_int(server, "port");
if (v.ok) cfg->port = (uint16_t)v.u.i;Values like difficulty -- cast respawn_time -- no validation. A negative value could cause immediate respawn or underflow. heartbeat_interval -- no validation. A value of 0 causes rapid-fire heartbeats flooding the master server. Recommended fix -- validate in v = toml_table_int(server, "port");
if (v.ok && v.u.i >= 1 && v.u.i <= 65535)
cfg->port = (uint16_t)v.u.i;
else if (v.ok)
fprintf(stderr, "config: invalid port %lld (must be 1-65535)\n", (long long)v.u.i);
v = toml_table_int(game, "difficulty");
if (v.ok && v.u.i >= 0 && v.u.i <= 2)
cfg->difficulty = (int)v.u.i;
v = toml_table_int(game, "respawn_time");
if (v.ok && v.u.i >= 0 && v.u.i <= 3600)
cfg->respawn_time = (int)v.u.i;
v = toml_table_int(master, "heartbeat_interval");
if (v.ok && v.u.i >= 10 && v.u.i <= 3600)
cfg->heartbeat_interval = (int)v.u.i;Severity: High -- invalid values silently propagate into server behavior with no warning. |
🔴 H2:
|
🟡 M1: Path traversal risk in DLL/Lua module paths (document now, enforce later)File: Config fields Current risk: LOW -- server.toml is a local admin-controlled file, and the Future risk: HIGH -- the Recommended action for this PR: Add a comment in config.h on the module struct: /* SECURITY: dll/lua paths will be used for code loading in A2.
* When the module loader is implemented, validate paths against
* an allowlist directory and reject traversal sequences (../). */
char dll[256];
char lua[256]; |
🟡 M2: Silent string truncation with no warningFile: static void str_copy(char *dst, size_t dstsz, const char *src)
{
if (!dst || dstsz == 0 || !src) return;
strncpy(dst, src, dstsz - 1);
dst[dstsz - 1] = '\0';
}Correct and safe (always NUL-terminates), but truncation is silent. A server name longer than 63 bytes, a path longer than 255 bytes, or a module key longer than 63 bytes is clipped with no indication to the operator. Recommended enhancement: static void str_copy(char *dst, size_t dstsz, const char *src)
{
if (!dst || dstsz == 0 || !src) return;
if (strlen(src) >= dstsz)
fprintf(stderr, "config: warning: value truncated to %zu chars: '%.32s...'\n",
dstsz - 1, src);
strncpy(dst, src, dstsz - 1);
dst[dstsz - 1] = '\0';
} |
🟡 M3: Capacity overflows silently truncate with no warningFile: When arrays exceed their maximums, excess entries are silently dropped:
The operator gets a working but incomplete config with no indication anything was dropped. Recommended fix: Add a warning after each clamping: int n = toml_array_len(modules);
if (n > OBC_CFG_MODULES_MAX) {
fprintf(stderr, "config: warning: %d modules exceeds max %d; extras ignored\n",
n, OBC_CFG_MODULES_MAX);
n = OBC_CFG_MODULES_MAX;
}Apply same pattern to masters, mod_packs, and per-module kv_count. |
🟡 M4: Add file size guard before TOML parsingFile: The vendored toml-c library reads the entire file into a heap buffer with no size limit. A symlinked Recommended fix: Add a size check before parsing: bool obc_config_load(const char *path, obc_server_cfg_t *cfg)
{
if (!path || !cfg) return false;
FILE *fp = fopen(path, "r");
if (!fp) return false;
/* Config files should be small; reject anything over 1 MB */
fseek(fp, 0, SEEK_END);
long sz = ftell(fp);
if (sz > 1024 * 1024) {
fprintf(stderr, "config: '%s' too large (%ld bytes, max 1MB)\n", path, sz);
fclose(fp);
return false;
}
rewind(fp);
// ... existing parse logic ...
} |
🟡 M5:
|
🟡 M6: Makefile CRLF-to-LF conversion inflates diffFile: The Makefile on
The CRLF-to-LF conversion is actually correct (LF is proper for Makefiles -- GNU Make can be picky about CRLF in recipes). But it should ideally be a separate commit so Recommendation: Consider adding a |
🔵 L1:
|
🔵 L2: Vendored toml-c placement in
|
🔵 L3: toml-c CALLOC uses
|
🔵 L4: Memory leak analysis -- CLEANI traced all v = toml_table_string(server, "name");
if (v.ok) { str_copy(cfg->name, sizeof(cfg->name), v.u.s); free(v.u.s); }All 14 string-access sites are correctly paired with No memory leaks found. |
📋 Review SummaryPR scope vs issue #124: All acceptance criteria met -- TOML parser integrated, server.toml loading replaces hardcoded globals, module config sections accessible via
Recommendation: Fix H1 (integer range validation) and H2 (strtol/strtod error checking) before merge. These are small changes that prevent silent misconfiguration from flowing into runtime behavior. The rest (M1-M6, L1-L4) can be follow-up. |
|
@Cadacious Addressed the latest PR #173 review round in commit Summary of fixes:
Test updates:
Validation run:
|
Re-review: PR #173 -- TOML Config System (Issue #124)SummaryPR #173 adds a TOML-based configuration system. Vendors toml-c (MIT), implements Clean-Room StatusPASS. No RE artifacts. All protocol constants ( Security
Game Correctness4 issues found: G1 -- MUST FIX: Master server accumulation breaks "CLI wins" contract. G2 -- MUST FIX: G3 -- MUST FIX: G4 -- MUST FIX: G5 -- 5 dead config fields. Code Quality
Suggestions
VerdictREQUEST CHANGES. Architecture is sound, memory management is clean, test coverage is strong. Four issues must be fixed:
All four are small, localized fixes (a few lines each). F2-F4 affect accessor functions no module consumes yet, but the API contract should be correct from day one. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/config.c`:
- Around line 132-138: The [game].system value is parsed into cfg->system
without enforcing the documented 1..9 range; modify the toml_table_int handling
after parse_i64_for_int to check that parsed_system is between 1 and 9, assign
cfg->system only on success, and call warn_invalid_i64 (or a new range-specific
warning) when the value is out of range so g_server_cfg.system cannot remain
invalid for other consumers; keep existing parse_i64_for_int and toml_table_int
usage and ensure you don't duplicate clamping done in main.c.
- Around line 454-464: The use of strdup in obc_config_load_str is not portable
under -std=c11; replace the strdup(src) call with an explicit malloc of
strlen(src)+1 followed by memcpy and manual null-termination, assigning that
buffer to the same local variable (buf) and keeping the existing free(buf)
usage; update obc_config_load_str to use this malloc+memcpy pattern (or the
project's existing portable string helper if available) so the toml_parse call
still receives a writable copy.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
include/openbc/config.hsrc/server/config.csrc/server/main.csrc/toml/toml.ctests/test_config.c
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_config.c`:
- Around line 1-5: Remove the unused header include <math.h> from the top of the
file (the include list that currently has "test_util.h" and "openbc/config.h");
it is not used by any functions or comparisons in this test (no fabs/sqrt/etc.),
so delete the line containing `#include` <math.h> to avoid unnecessary
dependencies and -Wpedantic noise.
- Around line 181-204: Add an explicit lower-bound check for port=1 in the TEST
function test_load_str_int_range_valid_boundaries by calling obc_config_load_str
with a TOML string where "[server]\nport = 1\n" (keep the other fields
identical) and asserting ASSERT(obc_config_load_str(toml, &cfg) == true)
followed by ASSERT_EQ_INT(1, (int)cfg.port); this can be done by either
replacing the existing TOML in test_load_str_int_range_valid_boundaries or
appending a second load/assert block in the same test to cover the lower
boundary while still validating cfg.system, cfg.difficulty, cfg.respawn_time,
and cfg.heartbeat_interval via their existing assertions.
| #include "test_util.h" | ||
| #include "openbc/config.h" | ||
|
|
||
| #include <string.h> | ||
| #include <math.h> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused #include <math.h>.
No math functions (fabs, sqrt, etc.) are used in this file. The float comparisons rely on relational operators. Consider removing this include to keep the dependency set minimal and avoid -Wpedantic noise if the header pulls in platform-specific extensions.
Proposed fix
`#include` "test_util.h"
`#include` "openbc/config.h"
`#include` <string.h>
-#include <math.h>🧰 Tools
🪛 Cppcheck (2.19.0)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_config.c` around lines 1 - 5, Remove the unused header include
<math.h> from the top of the file (the include list that currently has
"test_util.h" and "openbc/config.h"); it is not used by any functions or
comparisons in this test (no fabs/sqrt/etc.), so delete the line containing
`#include` <math.h> to avoid unnecessary dependencies and -Wpedantic noise.
| TEST(test_load_str_int_range_valid_boundaries) | ||
| { | ||
| obc_server_cfg_t cfg; | ||
| obc_config_defaults(&cfg); | ||
|
|
||
| const char *toml = | ||
| "[server]\n" | ||
| "port = 65535\n" | ||
| "\n" | ||
| "[game]\n" | ||
| "system = 9\n" | ||
| "difficulty = 0\n" | ||
| "respawn_time = 3600\n" | ||
| "\n" | ||
| "[master]\n" | ||
| "heartbeat_interval = 10\n"; | ||
|
|
||
| ASSERT(obc_config_load_str(toml, &cfg) == true); | ||
| ASSERT_EQ_INT(65535, (int)cfg.port); | ||
| ASSERT_EQ_INT(9, cfg.system); | ||
| ASSERT_EQ_INT(0, cfg.difficulty); | ||
| ASSERT_EQ_INT(3600, cfg.respawn_time); | ||
| ASSERT_EQ_INT(10, cfg.heartbeat_interval); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a lower-boundary test for port = 1.
The test validates the upper boundary (port = 65535) but not the lower boundary (port = 1). While port = 0 is implicitly tested via the range-rejection test (port must be 1..65535), an explicit port = 1 acceptance test would complete the boundary pair.
Add to test_load_str_int_range_valid_boundaries or as a separate assertion
ASSERT(obc_config_load_str(toml, &cfg) == true);
ASSERT_EQ_INT(65535, (int)cfg.port);
+
+ /* Also verify lower boundary */
+ obc_config_defaults(&cfg);
+ ASSERT(obc_config_load_str("[server]\nport = 1\n", &cfg) == true);
+ ASSERT_EQ_INT(1, (int)cfg.port);
+
ASSERT_EQ_INT(9, cfg.system);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_config.c` around lines 181 - 204, Add an explicit lower-bound
check for port=1 in the TEST function test_load_str_int_range_valid_boundaries
by calling obc_config_load_str with a TOML string where "[server]\nport = 1\n"
(keep the other fields identical) and asserting ASSERT(obc_config_load_str(toml,
&cfg) == true) followed by ASSERT_EQ_INT(1, (int)cfg.port); this can be done by
either replacing the existing TOML in test_load_str_int_range_valid_boundaries
or appending a second load/assert block in the same test to cover the lower
boundary while still validating cfg.system, cfg.difficulty, cfg.respawn_time,
and cfg.heartbeat_interval via their existing assertions.
…124) Vendor toml-c (MIT, arp242/toml-c) into src/toml/ and implement a layered configuration system: hardcoded defaults -> server.toml -> CLI args. Modules loaded in later phases will read per-module config via the obc_config_mod_* accessors on the global g_server_cfg. - src/toml/toml.c, toml.h, LICENSE: vendored toml-c library - include/openbc/config.h: obc_server_cfg_t, obc_module_cfg_t, full API - src/server/config.c: obc_config_defaults/load/load_str/free + mod accessors - include/openbc/server_state.h: expose g_server_cfg global - src/server/server_state.c: define g_server_cfg - src/server/main.c: load server.toml before CLI arg parsing - server.toml: stock default configuration file - LICENSE-THIRD-PARTY: MIT attribution for toml-c (Copyright CK Tan) - tests/test_config.c: 17 unit tests covering all API surfaces - Makefile: add -Isrc, TOML_OBJ, CONFIG_OBJ to SERVER_LIB_OBJ Closes #124 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Validate [game].system range 1..9 (was unchecked int) - Replace strdup with malloc+memcpy for C11 portability - Add system range tests to validation and boundary test cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6699347 to
fe12859
Compare
Summary
src/toml/toml.c+toml.h— no build-time dependency, zero external downloadsserver.toml→ CLI args; CLI always winsg_server_cfg(typeobc_server_cfg_t) as a server global; future module loaders (A2) and the engine API table (A3) will pass a pointer to it into loaded modulesChanges
src/toml/toml.c,toml.h,LICENSEinclude/openbc/config.hobc_server_cfg_t,obc_module_cfg_t, full public APIsrc/server/config.cobc_config_defaults/load/load_str/free+obc_config_mod_string/int/float/boolinclude/openbc/server_state.hextern obc_server_cfg_t g_server_cfgsrc/server/server_state.cg_server_cfgsrc/server/main.cserver.tomlbefore CLI arg parsingserver.tomlLICENSE-THIRD-PARTYtests/test_config.cMakefile-Isrcflag;TOML_OBJ+CONFIG_OBJadded toSERVER_LIB_OBJTesting
New test suite
test_configcovers:obc_config_defaults()values match documented defaultsobc_config_load_str()for every TOML section ([server],[game],[data],[gamespy],[master],[[modules]])falsesilentlyfalseobc_config_mod_{string,int,float,bool}()found/missing key/missing module pathsobc_config_free()no-op safety checkNotes
obc_server_cfg_t.nameand.mapare capped at 64 bytes to match the GameSpyhostname/missionscriptwire fields — prevents-Wformat-truncationwarnings from themain.csnprintf calls that copy these intog_info.obc_config_mod_*accessors without requiring callers to know the original type.[[modules]]entries withlua = "..."are parsed and stored; the Lua runtime (A14) will consume them.Closes #124
Summary by CodeRabbit
New Features
Documentation
Tests