Skip to content

Fix invalid JSON dump for objects without debug properties - #142

Open
ousamabenyounes wants to merge 1 commit into
BitOne:masterfrom
ousamabenyounes:fix/issue-104
Open

Fix invalid JSON dump for objects without debug properties#142
ousamabenyounes wants to merge 1 commit into
BitOne:masterfrom
ousamabenyounes:fix/issue-104

Conversation

@ousamabenyounes

Copy link
Copy Markdown

Fix #104

Summary

meminfo_dump() writes "object_handle" : "N", with an unconditional trailing
comma, then only writes the "children" block when the object exposes debug
properties. An object whose debug handler exposes nothing therefore ends on a
dangling comma and the whole dump stops being parseable:

        "class" : "FFI\\CType",
        "object_handle" : "1",

    },

That is exactly the output pasted in the issue, and json_decode() on the dump
returns JSON_ERROR_SYNTAX, so the analyzer cannot load the file at all.

Root cause

meminfo_zval_dump() resolves properties with
zend_get_properties_for(zv, ZEND_PROP_PURPOSE_DEBUG). That purpose supersedes
the get_debug_info handler, and a handler is allowed to return NULL
FFI\CType does exactly that. The separator was written before knowing whether
anything would follow it.

The fix writes "object_handle" without a separator and lets each branch emit
its own: ,\n when the "children" block follows, \n when it does not. This
mirrors the existing scalar branch a few lines below, so the emitted bytes are
unchanged for every object that does expose properties.

The NULL case is already the documented shape on the reader side — every
analyzer consumer guards with isset($item['children']) — so nothing else has
to change.

Test verification (RED → GREEN)

New test: extension/tests/bug-github-104_ffi_object_invalid_json.phpt, in the
existing dump-check_json*.phpt idiom. It SKIPIFs without ext/FFI (which only
exists from PHP 7.4 on).

RED — unmodified master (0ab7f5a), new test applied on its own, no production change:

TEST 1/1 [tests/bug-github-104_ffi_object_invalid_json.phpt] FAIL
Check that an object without debug properties (FFI\CType) does not break the JSON dump

Number of tests :    1                 1
Tests failed    :    1 (100.0%) (100.0%)
Tests passed    :    0 (  0.0%) (  0.0%)

with the expected-vs-actual diff:

001+ meminfo_dump JSON decode fail: Syntax error
001- meminfo_dump JSON decode ok
002- FFI\CType item present: yes

GREEN — this branch, full .phpt suite on PHP 8.1:

TEST  1/14 [tests/bug-github-104_ffi_object_invalid_json.phpt] PASS
TEST  2/14 [tests/bug-github-49_escape_control_character.phpt] PASS
TEST  3/14 [tests/bug-github-63_php7_wrong_exec_fame.phpt] PASS
TEST  4/14 [tests/bug-github-68_duplicate_objects_php7.phpt] PASS
TEST  5/14 [tests/bug-github-70_duplicated_referenced_scalar_php7.phpt] PASS
TEST  6/14 [tests/bug-github-75_missing_static_members.phpt] PASS
TEST  7/14 [tests/bug-github-76_children_items_not_linked_php7.phpt] PASS
TEST  8/14 [tests/dump-array.phpt] PASS
TEST  9/14 [tests/dump-check_json.phpt] PASS
TEST 10/14 [tests/dump-check_json_escape.phpt] PASS
TEST 11/14 [tests/dump-check_json_unicode.phpt] PASS
TEST 12/14 [tests/dump-memory_leak.phpt] PASS
TEST 13/14 [tests/dump-scalar.phpt] PASS
TEST 14/14 [tests/segfault-shutdown.phpt] PASS

Number of tests :   14                14
Tests failed    :    0 (  0.0%) (  0.0%)
Tests passed    :   14 (100.0%) (100.0%)

A third run with the production hunk reverted but the test kept still fails, so
the test is tied to the fix and not to the environment.

Full local suite

The complete local validation suite was replayed in Docker — every job class of
.github/workflows/build.yaml, on the whole matrix
(phpize && ./configure && make && make test)
(PHP 7.0, 7.1, 7.2, 7.3, 7.4, 8.0, 8.1) plus
cd analyzer && composer install && vendor/bin/phpspec run.

PHP build .phpt suite
7.0 ok 13 passed, 1 skipped (no ext/FFI before 7.4)
7.1 ok 13 passed, 1 skipped (no ext/FFI before 7.4)
7.2 ok 13 passed, 1 skipped (no ext/FFI before 7.4)
7.3 ok 13 passed, 1 skipped (no ext/FFI before 7.4)
7.4 ok 13 passed, 1 skipped (ext/FFI not installed in that image)
8.0 ok 13 passed, 1 skipped (ext/FFI not installed in that image)
8.1 ok 14 passed, 0 skipped — the new test runs here
analyzer ok phpspec: 6 examples, 6 passed

On master the same replay is 13 passed / 0 failed on every version, so no
previously green test changed state and the passing count never drops.

Line coverage of the changed C lines, measured with gcov over the .phpt
suite: 100.00% (3/3) added executable lines covered.

Note on CI: this repo's workflow only triggers on: push and pins the retired
ubuntu-20.04 runner, so pull requests here get an empty check list — that is
pre-existing and not specific to this PR, which is why the replay above is run
locally.

Note for reviewers

Open PR #136 touches the immediately adjacent lines (it changes which
properties are fetched for the dump). It is not a duplicate of this one — it
still needs this NULL guard — but whichever lands second will need a small
textual rebase.

Files changed

File Change
extension/meminfo.c Emit the "object_handle" separator only when a "children" block actually follows
extension/tests/bug-github-104_ffi_object_invalid_json.phpt Regression test: dumping an FFI\CType must produce decodable JSON

meminfo_dump() always wrote a comma after "object_handle", but the
"children" block meant to follow it is only emitted when the object's
debug handler exposes properties. FFI\CType exposes none, so such a
dump ended on a dangling comma and json_decode() rejected the whole
file.

Emit the separator from the branch that actually writes children, and a
plain newline otherwise, mirroring the scalar branch below.

Generated by Ora Studio
Vibe coded by ousamabenyounes

Co-Authored-By: Ora Agent <noreply@oratelecom.net>
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.

Invalid JSON for FFI objects

1 participant