From c6ab9650625b328751d030a4b1ce0694f6e60aea Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:30:43 +0800 Subject: [PATCH] fix(python): preserve historical signed code fields Keep signed co_stacksize and co_flags for historical magic entries whose constructors accepted them. Preserve magic 3410 with a constructor-revision limitation; reject negative values from 3411 onward. Fixes #6487 --- docs/python-pyc.md | 22 +++- src/domain/python/pythonPyc.test.ts | 9 ++ src/domain/python/pythonPyc.ts | 20 ++- src/python/PythonPycParser.test.ts | 24 +++- src/python/PythonPycParser.ts | 18 ++- tests/acceptance/pythonPycCli.test.ts | 156 +++++++++++++++--------- tests/boundary/mcp/pythonPycMcp.test.ts | 83 +++++++++++-- 7 files changed, 253 insertions(+), 79 deletions(-) diff --git a/docs/python-pyc.md b/docs/python-pyc.md index 997ce574a6..614c87aa09 100644 --- a/docs/python-pyc.md +++ b/docs/python-pyc.md @@ -78,10 +78,24 @@ and a null dictionary value ends the dict without that entry. The selected file is limited to 16 MiB by default (64 MiB maximum). Marshal object count, nesting depth, and per-string size are bounded; exceeding a budget fails without partial success. Argument counts, positional-only counts, -keyword-only counts, local counts, and stack size are signed CPython fields; a -negative encoding is malformed. Code `flags` preserve signed `co_flags` values; -CPython 3.8+ rejects negative flags, while 3.7 accepts them. On the 3.11+ -layout, `co_code` must cover a +keyword-only counts, and local counts must be non-negative. `co_stacksize` and +`co_flags` are retained as signed 32-bit source fields when the selected CPython +constructor accepted them. CPython 3.7 and 3.8 alpha constructors through +3.8.0a4 did not reject negative stack sizes or flags; the check was added by +[CPython commit a9f05d69](https://github.com/python/cpython/commit/a9f05d69ccbf3c75cdd604c25094282697789a62). +Magic 3410 is ambiguous: 3.8.0a4 and 3.8.0b1 both use it, but they are on +opposite sides of that constructor change. Negative values under magic 3410 +are preserved with an explicit limitation because the header cannot identify +which constructor revision produced them. Magic 3411 and later 3.8 magics were +introduced after that change, so their constructors reject negative values. +The pinned magic entries distinguish these cases; see the CPython +[3.7.0 constructor](https://github.com/python/cpython/blob/v3.7.0/Objects/codeobject.c), +[3.8.0a4 constructor](https://github.com/python/cpython/blob/v3.8.0a4/Objects/codeobject.c), +[3.8.0b1 constructor](https://github.com/python/cpython/blob/v3.8.0b1/Objects/codeobject.c), +and [3.8.0b2 magic table](https://github.com/python/cpython/blob/v3.8.0b2/Lib/importlib/_bootstrap_external.py). +Complete marshal coverage means all fields in the selected file were decoded; +it does not establish that CPython will accept or execute the code object. On +the 3.11+ layout, `co_code` must cover a whole number of code units. Every layout rejects argument counts that do not fit the local table CPython checks (`co_varnames`, including `*args` / `**kwargs`, or fast locals on 3.11+). Those files are malformed, so the diff --git a/src/domain/python/pythonPyc.test.ts b/src/domain/python/pythonPyc.test.ts index 1c7bc34c26..4345d0427b 100644 --- a/src/domain/python/pythonPyc.test.ts +++ b/src/domain/python/pythonPyc.test.ts @@ -10,6 +10,7 @@ it("maps pinned CPython 3.7–3.13 magics and leaves gaps unknown", () => { python_version: "3.7", code_layout: "py37", header_bytes: 16, + negative_code_fields: "accepted", }); expect(pythonPycMagicEntry(3390)).toMatchObject({ python_version: "3.7", @@ -23,16 +24,24 @@ it("maps pinned CPython 3.7–3.13 magics and leaves gaps unknown", () => { python_version: "3.8", code_layout: "py38", header_bytes: 16, + negative_code_fields: "rejected", }); + expect(pythonPycMagicEntry(3401)?.negative_code_fields).toBe("accepted"); + expect(pythonPycMagicEntry(3410)?.negative_code_fields).toBe( + "revision-dependent", + ); + expect(pythonPycMagicEntry(3411)?.negative_code_fields).toBe("rejected"); expect(pythonPycMagicEntry(3531)).toEqual({ python_version: "3.12", code_layout: "py311", header_bytes: 16, + negative_code_fields: "rejected", }); expect(pythonPycMagicEntry(3571)).toEqual({ python_version: "3.13", code_layout: "py311", header_bytes: 16, + negative_code_fields: "rejected", }); expect(pythonPycMagicEntry(3450)).toBeNull(); expect(pythonPycMagicEntry(62171)).toBeNull(); diff --git a/src/domain/python/pythonPyc.ts b/src/domain/python/pythonPyc.ts index d55167cfe6..6f4169c94a 100644 --- a/src/domain/python/pythonPyc.ts +++ b/src/domain/python/pythonPyc.ts @@ -20,6 +20,8 @@ type MagicEntry = { readonly python_version: string; readonly code_layout: PythonPycCodeLayout; readonly header_bytes: 12 | 16; + /** CPython constructor policy for negative co_stacksize/co_flags fields. */ + readonly negative_code_fields: "accepted" | "revision-dependent" | "rejected"; }; const magics = ( @@ -27,8 +29,12 @@ const magics = ( python_version: string, code_layout: PythonPycCodeLayout, header_bytes: 12 | 16 = PYTHON_PYC_HEADER_BYTES, + negative_code_fields: MagicEntry["negative_code_fields"] = "rejected", ): ReadonlyArray => - values.map((magic) => [magic, { python_version, code_layout, header_bytes }]); + values.map((magic) => [ + magic, + { python_version, code_layout, header_bytes, negative_code_fields }, + ]); const consecutive = ( start: number, @@ -48,10 +54,12 @@ const consecutive = ( * bytecode opcode tables. */ export const PYTHON_PYC_MAGIC_TABLE: ReadonlyMap = new Map([ - ...magics([3390, 3391], "3.7", "py37", 12), - ...magics([3392, 3393, 3394], "3.7", "py37"), - ...magics([3400, 3401], "3.8", "py37"), - ...magics([3410, 3411, 3412, 3413], "3.8", "py38"), + ...magics([3390, 3391], "3.7", "py37", 12, "accepted"), + ...magics([3392, 3393, 3394], "3.7", "py37", 16, "accepted"), + ...magics([3400, 3401], "3.8", "py37", 16, "accepted"), + // 3410 spans 3.8.0a4 and b1; a9f05d69 added constructor sign checks between. + ...magics([3410], "3.8", "py38", 16, "revision-dependent"), + ...magics([3411, 3412, 3413], "3.8", "py38", 16, "rejected"), ...magics([3420, 3421, 3422, 3423, 3424, 3425], "3.9", "py38"), ...magics( [3430, 3431, 3432, 3433, 3434, 3435, 3436, 3437, 3438, 3439], @@ -166,7 +174,7 @@ const pythonCodeFields = { posonlyargcount: nonNegativeInt32.nullable(), kwonlyargcount: nonNegativeInt32, nlocals: nonNegativeInt32.nullable(), - stacksize: nonNegativeInt32, + stacksize: signedInt32, flags: signedInt32, names: z.array(z.string()), varnames: z.array(z.string()).nullable(), diff --git a/src/python/PythonPycParser.test.ts b/src/python/PythonPycParser.test.ts index 1aeca3c944..4fea1c6ca7 100644 --- a/src/python/PythonPycParser.test.ts +++ b/src/python/PythonPycParser.test.ts @@ -1520,9 +1520,12 @@ it("rejects code CPython treats as corrupt instead of complete coverage", () => expect(() => parse(file("py311", 3531, { flags: 0x80000000 }))).toThrow( /co_flags is negative/, ); - expect(() => parse(file("py37", 3401, { flags: 0x80000000 }))).toThrow( + expect(() => parse(file("py38", 3411, { flags: 0x80000000 }))).toThrow( /co_flags is negative/, ); + expect(() => parse(file("py38", 3411, { stacksize: 0xffffffff }))).toThrow( + /co_stacksize is negative/, + ); const valid = parse( file("py311", 3531, { @@ -1544,6 +1547,25 @@ it("rejects code CPython treats as corrupt instead of complete coverage", () => const negativePy37Flags = parse(file("py37", 3394, { flags: 0x80000000 })); expect(root(negativePy37Flags).flags).toBe(-0x80000000); expect(pythonPycSchema.safeParse(negativePy37Flags).success).toBe(true); + const negativePy37Stacksize = parse( + file("py37", 3394, { stacksize: 0xffffffff }), + ); + expect(root(negativePy37Stacksize).stacksize).toBe(-1); + expect(pythonPycSchema.safeParse(negativePy37Stacksize).success).toBe(true); + const negativePy38AlphaFlags = parse( + file("py37", 3401, { flags: 0x80000000 }), + ); + expect(root(negativePy38AlphaFlags).flags).toBe(-0x80000000); + const ambiguousPy38Fields = parse( + file("py38", 3410, { stacksize: 0xffffffff, flags: 0x80000000 }), + ); + expect(root(ambiguousPy38Fields)).toMatchObject({ + stacksize: -1, + flags: -0x80000000, + }); + expect(ambiguousPy38Fields.limitations).toEqual( + expect.arrayContaining([expect.stringContaining("Magic 3410 is shared")]), + ); const py37 = parse( file("py37", 3394, { argcount: 1, diff --git a/src/python/PythonPycParser.ts b/src/python/PythonPycParser.ts index 394b4a02aa..2c245241eb 100644 --- a/src/python/PythonPycParser.ts +++ b/src/python/PythonPycParser.ts @@ -235,7 +235,7 @@ export const parsePythonPyc = ( marshal, mapped.code_layout, input, - mapped.python_version !== "3.7", + mapped.negative_code_fields === "rejected", ); const root = reader.readObject(0); if (reader.offset !== marshal.length) @@ -268,6 +268,12 @@ export const parsePythonPyc = ( ? "source_hash_hex is the PEP 552 header field; the original .py is not read." : "source_mtime and source_size are header fields; they do not prove a matching .py is present.", "python_version is the pinned CPython magic table for this header, not a runtime interpreter.", + ...(mapped.negative_code_fields === "revision-dependent" && + codeObjects.some((code) => code.stacksize < 0 || code.flags < 0) + ? [ + "Magic 3410 is shared by CPython revisions with different negative co_stacksize/co_flags constructor checks; these raw values are preserved, but this header cannot establish constructor acceptance.", + ] + : []), ...(unknownFlags === 0 ? [] : [ @@ -323,7 +329,7 @@ class MarshalReader { readonly bytes: Buffer, readonly layout: PythonPycCodeLayout, readonly input: InspectPythonPycInput, - readonly rejectNegativeCodeFlags: boolean, + readonly rejectNegativeCodeFields: boolean, ) {} readObject(depth: number): MarshalValue { @@ -437,9 +443,11 @@ class MarshalReader { const kwonlyargcount = this.nonNegativeInt32("co_kwonlyargcount"); const nlocals = this.layout === "py311" ? null : this.nonNegativeInt32("co_nlocals"); - const stacksize = this.nonNegativeInt32("co_stacksize"); + const stacksize = this.i32(); + if (this.rejectNegativeCodeFields && stacksize < 0) + malformed("co_stacksize is negative."); const flags = this.i32(); - if (this.rejectNegativeCodeFlags && flags < 0) + if (this.rejectNegativeCodeFields && flags < 0) malformed("co_flags is negative."); const bytecode = asBytes(this.readObject(depth + 1), "co_code"); // 3.11+ `_PyCode_Validate` rejects a length that is not a whole code unit. @@ -1501,7 +1509,7 @@ class MarshalReader { return value; } - /** CPython `r_long` int fields must not be negative in a code object. */ + /** CPython `r_long` count fields that its constructors require non-negative. */ nonNegativeInt32(field: string): number { const value = this.i32(); if (value < 0) malformed(`${field} is negative.`); diff --git a/tests/acceptance/pythonPycCli.test.ts b/tests/acceptance/pythonPycCli.test.ts index 9be04c7916..d2ab1c7584 100644 --- a/tests/acceptance/pythonPycCli.test.ts +++ b/tests/acceptance/pythonPycCli.test.ts @@ -126,84 +126,130 @@ cliTest( ); cliTest( - "rejects constructor-invalid signed code flags and preserves unknown positive flags", + "preserves historical signed code fields and reports the ambiguous 3410 constructor revision", async ({ cli }) => { - const root = await createTestTempDirectory("rea-python-pyc-flags-"); - for (const { name, magic, layout, flags, expected, valid } of [ + const root = await createTestTempDirectory("rea-python-pyc-signed-fields-"); + for (const { name, magic, layout, fields, expected } of [ { name: "zero", magic: 3531, layout: "py311", - flags: 0, - expected: 0, - valid: true, + fields: { flags: 0 }, + expected: { stacksize: 1, flags: 0 }, }, { - name: "unknown-positive", + name: "unknown-positive-flags", magic: 3531, layout: "py311", - flags: 0x40000000, - expected: 0x40000000, - valid: true, + fields: { flags: 0x40000000 }, + expected: { stacksize: 1, flags: 0x40000000 }, }, { - name: "negative-py311", - magic: 3531, - layout: "py311", - flags: 0x80000000, - expected: -0x80000000, - valid: false, - }, - { - name: "negative-plus-one-py311", - magic: 3531, - layout: "py311", - flags: 0x80000001, - expected: -0x7fffffff, - valid: false, - }, - { - name: "negative-py37", + name: "negative-stacksize-py37", magic: 3394, layout: "py37", - flags: 0x80000000, - expected: -0x80000000, - valid: true, + fields: { stacksize: 0xffffffff }, + expected: { stacksize: -1, flags: 0 }, + }, + { + name: "negative-flags-py38-alpha", + magic: 3401, + layout: "py37", + fields: { flags: 0x80000000 }, + expected: { stacksize: 1, flags: -0x80000000 }, + }, + { + name: "negative-stack-and-flags-py38-ambiguous", + magic: 3410, + layout: "py38", + fields: { stacksize: 0xffffffff, flags: 0x80000000 }, + expected: { stacksize: -1, flags: -0x80000000 }, }, ] as const) { const path = join(root, `${name}.pyc`); await writeFile( path, - Buffer.concat([pythonPycHeader(magic), marshalCode(layout, { flags })]), + Buffer.concat([pythonPycHeader(magic), marshalCode(layout, fields)]), ); const result = await cli.run({ arguments: ["inspect-python-pyc", path, "--json"], }); - if (valid) { - expect(result.exitCode, result.stderr).toBe(0); - const evidence = toolContract("inspect_python_pyc").outputSchema.parse( - result.json, + expect(result.exitCode, result.stderr).toBe(0); + const evidence = toolContract("inspect_python_pyc").outputSchema.parse( + result.json, + ); + expect(evidence.normalized_result.code_objects[0]).toMatchObject( + expected, + ); + if (magic === 3410) + expect(evidence.normalized_result.limitations).toEqual( + expect.arrayContaining([ + expect.stringContaining("Magic 3410 is shared"), + ]), ); - expect(evidence.normalized_result.code_objects[0]?.flags).toBe( - expected, - ); - } else { - expect(result.exitCode).toBe(1); - expect(result.json).toMatchObject({ - code: "invalid_request", - category: "invalid_input", - details: { - operation: "inspect_python_pyc", - issues: [ - { - reason: "invalid_format", - message: expect.stringContaining("co_flags is negative"), - }, - ], - }, - }); - expect(result.json).not.toHaveProperty("normalized_result"); - } + } + }, +); + +cliTest( + "rejects negative code fields after the pinned constructor validation transition", + async ({ cli }) => { + const root = await createTestTempDirectory( + "rea-python-pyc-signed-rejection-", + ); + for (const { name, magic, layout, fields, message } of [ + { + name: "negative-stacksize-py38-b2", + magic: 3411, + layout: "py38", + fields: { stacksize: 0xffffffff }, + message: "co_stacksize is negative", + }, + { + name: "negative-flags-py38-b2", + magic: 3411, + layout: "py38", + fields: { flags: 0x80000000 }, + message: "co_flags is negative", + }, + { + name: "negative-stacksize-py311", + magic: 3531, + layout: "py311", + fields: { stacksize: 0xffffffff }, + message: "co_stacksize is negative", + }, + { + name: "negative-flags-py311", + magic: 3531, + layout: "py311", + fields: { flags: 0x80000000 }, + message: "co_flags is negative", + }, + ] as const) { + const path = join(root, `${name}.pyc`); + await writeFile( + path, + Buffer.concat([pythonPycHeader(magic), marshalCode(layout, fields)]), + ); + const result = await cli.run({ + arguments: ["inspect-python-pyc", path, "--json"], + }); + expect(result.exitCode).toBe(1); + expect(result.json).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + details: { + operation: "inspect_python_pyc", + issues: [ + { + reason: "invalid_format", + message: expect.stringContaining(message), + }, + ], + }, + }); + expect(result.json).not.toHaveProperty("normalized_result"); } }, ); diff --git a/tests/boundary/mcp/pythonPycMcp.test.ts b/tests/boundary/mcp/pythonPycMcp.test.ts index 4f41dae436..3cd1084066 100644 --- a/tests/boundary/mcp/pythonPycMcp.test.ts +++ b/tests/boundary/mcp/pythonPycMcp.test.ts @@ -101,7 +101,7 @@ mcpTest("requires an absolute path for inspect_python_pyc", async ({ mcp }) => { }); mcpTest( - "advertised pyc output schema accepts signed legacy flags and positive unknown flags", + "advertised pyc output schema preserves historical signed fields", async ({ mcp }) => { const { client, session } = await connect(mcp); try { @@ -118,15 +118,29 @@ mcpTest( name: "legacy-negative", magic: 3394, layout: "py37" as const, - rawFlags: 0x80000000, - expectedFlags: -0x80000000, + fields: { stacksize: 0xffffffff }, + expected: { stacksize: -1, flags: 0 }, + }, + { + name: "alpha-3401-negative-flags", + magic: 3401, + layout: "py37" as const, + fields: { flags: 0x80000000 }, + expected: { stacksize: 1, flags: -0x80000000 }, + }, + { + name: "ambiguous-3410-negative-fields", + magic: 3410, + layout: "py38" as const, + fields: { stacksize: 0xffffffff, flags: 0x80000000 }, + expected: { stacksize: -1, flags: -0x80000000 }, }, { name: "unknown-positive", magic: 3531, layout: "py311" as const, - rawFlags: 0x40000000, - expectedFlags: 0x40000000, + fields: { flags: 0x40000000 }, + expected: { flags: 0x40000000 }, }, ]) { const path = join(root, `${testCase.name}.pyc`); @@ -134,7 +148,7 @@ mcpTest( path, Buffer.concat([ pythonPycHeader(testCase.magic), - marshalCode(testCase.layout, { flags: testCase.rawFlags }), + marshalCode(testCase.layout, testCase.fields), ]), ); const response = await client.callTool({ @@ -151,9 +165,62 @@ mcpTest( ajv.validate(outputSchema, response.structuredContent), JSON.stringify(ajv.errors), ).toBe(true); - expect(evidence.normalized_result.code_objects[0]?.flags).toBe( - testCase.expectedFlags, + expect(evidence.normalized_result.code_objects[0]).toMatchObject( + testCase.expected, ); + if (testCase.magic === 3410) + expect(evidence.normalized_result.limitations).toEqual( + expect.arrayContaining([ + expect.stringContaining("Magic 3410 is shared"), + ]), + ); + } + } finally { + await session.close(); + } + }, +); + +mcpTest( + "rejects negative code fields for magics after constructor validation was added", + async ({ mcp }) => { + const { client, session } = await connect(mcp); + try { + const root = await createTestTempDirectory("rea-python-pyc-mcp-signed-"); + for (const { name, fields, message } of [ + { + name: "negative-stacksize", + fields: { stacksize: 0xffffffff }, + message: "co_stacksize is negative", + }, + { + name: "negative-flags", + fields: { flags: 0x80000000 }, + message: "co_flags is negative", + }, + ]) { + const path = join(root, `${name}.pyc`); + await writeFile( + path, + Buffer.concat([pythonPycHeader(3411), marshalCode("py38", fields)]), + ); + const response = await client.callTool({ + name: "inspect_python_pyc", + arguments: { path }, + }); + expect(response.isError).toBe(true); + expect(parseMcpToolError(response).error).toMatchObject({ + code: "invalid_request", + details: { + operation: "inspect_python_pyc", + issues: [ + { + reason: "invalid_format", + message: expect.stringContaining(message), + }, + ], + }, + }); } } finally { await session.close();