Skip to content

mcp: stop int tool args from wrapping on 64-bit values - #162

Open
dafeiyupola wants to merge 2 commits into
octacity-org:mainfrom
dafeiyupola:fix/mcp-int-arg-range
Open

dafeiyupola wants to merge 2 commits into
octacity-org:mainfrom
dafeiyupola:fix/mcp-int-arg-range

Conversation

@dafeiyupola

Copy link
Copy Markdown

Problem

cbm_mcp_get_int_arg read every JSON integer with yyjson_get_int, which
truncates the 64-bit value to int. limit: 4294967297 therefore arrived as
1, and 2147483648 as a negative number that then reached code assuming a
non-negative count.

Reproduced against the current main with a small harness around the real
yyjson vendored source:

argument before after
4294967297 1 default
2147483648 -2147483648 default
-2147483649 2147483647 default
2147483647 2147483647 2147483647
-2147483648 -2147483648 -2147483648

Fix

Read with yyjson_get_sint and fall back to default_val when the value is
outside INT_MIN..INT_MAX, matching upstream #2256.

Tests

Adds mcp_get_int_arg_out_of_range to tests/test_mcp.c, covering both
overflowing directions and the in-range boundaries.

Note: I could not run make -f Makefile.cbm test-mcp locally because it
requires Zig 0.17, which is not installed here and the download would not
complete. The value-level behaviour above was verified with a standalone
harness compiled against the repo's own vendored/yyjson; CI will provide the
real build.

@ata-sesli ata-sesli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up. The cbm_mcp_get_int_arg change fixes the cases in the table, and the test is a good one. A few things are needed before this can merge.

1. The second site from #128 is not fixed

This PR fixes #128, which also points at src/mcp/mcp.c:1574 ("The same pattern exists ... check it too"). That line is in compare_parse_bounded_integer, which parses limit and scan_limit for compare_graphs:

parsed = yyjson_get_int(value);
if (parsed < 1 || parsed > maximum) { ... }

yyjson_get_int truncates before the range check runs. So limit: 4294967297 becomes 1, passes the 1..1000 check, and runs as limit 1 when it should be rejected with "limit must be between 1 and 1000".

Fix: parsed = yyjson_get_sint(value);. The int64_t range check after it then handles every case. Please also add a compare_graphs test that limit: 4294967297 is rejected.

2. Values just below 2^64 still wrap (inline comment)

See the comment on yyjson_get_sint.

3. PR housekeeping

  • Sign-off: neither commit has a Signed-off-by: line, and CI rejects unsigned commits (see CONTRIBUTING.md). git rebase --signoff main and a force-push will fix it. Squashing the two commits into one would also help, since they share a title.
  • Issue link: CONTRIBUTING.md asks every PR to reference its issue. Please add Fixes #128 to the description.
  • Template: please use the PR template checklist (.github/PULL_REQUEST_TEMPLATE.md).

Heads-up: overlap with #163

#163 ports src/mcp to Zig and deletes src/mcp/mcp.c. If #163 merges first, this change has to move to cbm_mcp_get_int_arg and the compare_graphs argument parser in src/mcp/*.zig. If this PR merges first, #163 will carry it over. Nothing to do yet, just so the conflict isn't a surprise.

The other yyjson_get_int calls in mcp.c (lines 153, 303 and 5775) handle the JSON-RPC id and output, not tool arguments. They are outside #128, so leaving them alone is right.

Comment thread src/mcp/mcp.c
/* Read the JSON integer as 64-bit and fall back to the default when it
* does not fit an int. Casting first wraps silently: limit 4294967297
* became 1, and 2147483648 became negative. */
int64_t wide = yyjson_get_sint(val);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values just below 2^64 still slip through. yyjson stores positive integers as unsigned 64-bit, and yyjson_get_sint reads the same bits as signed (vendored/yyjson/yyjson.h:5068-5070, 5369-5371). So {"limit": 18446744073709551615} gives wide == -1, which passes this check and returns -1. Every value from 2^64 - 2^31 to 2^64 - 1 comes back as a negative int, the same outcome #128 describes.

Suggested fix: check the subtype first.

if (yyjson_is_uint(val)) {
    uint64_t u = yyjson_get_uint(val);
    if (u <= (uint64_t)INT_MAX) {
        result = (int)u;
    }
} else {
    int64_t wide = yyjson_get_sint(val);
    if (wide >= (int64_t)INT_MIN && wide <= (int64_t)INT_MAX) {
        result = (int)wide;
    }
}

Please also add 18446744073709551615 to mcp_get_int_arg_out_of_range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants