weaselab/weaseljson
2
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Handle null parser in WeaselJsonParser_parse #54

Merged
andrew merged 3 commits from null-parser into main 2026-07-15 22:04:01 +00:00
Conversation 2 Commits 3 Files Changed 5
+14 -1
5 changed files with 14 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
include/weaseljson.h
+3 -1
View File
@@ -38,6 +38,8 @@ enum WeaselJsonStatus {
WeaselJson_REJECT, WeaselJson_REJECT,
/** json is too deeply nested */ /** json is too deeply nested */
WeaselJson_OVERFLOW, WeaselJson_OVERFLOW,
/** Tried to call parse on a null parser */
WeaselJson_NULL,
andrew marked this conversation as resolved
weaselbot commented 2026-07-15 18:01:40 +00:00
Review
Copy Link
Copy Source

Adding a new member to this public C ABI enum is the most impactful part of this PR. Issue #51 suggested returning WeaselJson_REJECT (a null parser can never accept input), which would fix the crash without changing the ABI. Since the project compiles with -Werror=switch-enum -Wswitch-enum, any switch over WeaselJsonStatus now has to handle this new case (hence the validate.cpp change), and external consumers using -Wswitch/-Werror will break on upgrade. If a distinct status is desired for diagnostics that's fine, but consider reusing WeaselJson_REJECT to avoid an ABI bump - or at least call out in the header that this is a newly-added status callers must handle.

Adding a new member to this public C ABI enum is the most impactful part of this PR. Issue #51 suggested returning WeaselJson_REJECT (a null parser can never accept input), which would fix the crash without changing the ABI. Since the project compiles with -Werror=switch-enum -Wswitch-enum, any switch over WeaselJsonStatus now has to handle this new case (hence the validate.cpp change), and external consumers using -Wswitch/-Werror will break on upgrade. If a distinct status is desired for diagnostics that's fine, but consider reusing WeaselJson_REJECT to avoid an ABI bump - or at least call out in the header that this is a newly-added status callers must handle.
andrew commented 2026-07-15 18:07:22 +00:00
Review
Copy Link
Copy Source

WeaselJson_REJECT would mean that the json was invalid. We need something semantically different.

WeaselJson_REJECT would mean that the json was invalid. We need something semantically different.
}; };
typedef struct WeaselJsonParser WeaselJsonParser; typedef struct WeaselJsonParser WeaselJsonParser;
@@ -65,7 +67,7 @@ void WeaselJsonParser_destroy(WeaselJsonParser *parser);
/** Incrementally parse `len` more bytes starting at `buf`. `buf` may be /** Incrementally parse `len` more bytes starting at `buf`. `buf` may be
* modified. Call with `len` 0 to indicate end of data. `buf` may be null if * modified. Call with `len` 0 to indicate end of data. `buf` may be null if
* `len` is 0. `len` must not be negative; a negative length is treated as a * `len` is 0. `len` must not be negative; a negative length is treated as a
* rejected input. */ * rejected input. Returns WeaselJson_NULL if parser is null */
andrew marked this conversation as resolved Outdated
weaselbot commented 2026-07-15 18:01:40 +00:00
Outdated
Review
Copy Link
Copy Source

The doc comment covers the negative-length behavior but not the null-parser behavior added in this PR. Please add a sentence, e.g. "If parser is null, parse returns WeaselJson_NULL (or WeaselJson_REJECT) without dereferencing it.", so the header stays self-consistent with the implementation.

The doc comment covers the negative-length behavior but not the null-parser behavior added in this PR. Please add a sentence, e.g. "If parser is null, parse returns WeaselJson_NULL (or WeaselJson_REJECT) without dereferencing it.", so the header stays self-consistent with the implementation.
WeaselJsonStatus WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf, WeaselJsonStatus WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf,
int len); int len);
src/lib.cpp
+3
View File
@@ -46,6 +46,9 @@ WeaselJsonParser_destroy(WeaselJsonParser *parser) {
__attribute__((visibility("default"))) WeaselJsonStatus __attribute__((visibility("default"))) WeaselJsonStatus
WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf, int len) { WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf, int len) {
if (parser == nullptr) [[unlikely]] {
return WeaselJson_NULL;
}
return ((Parser3 *)parser)->parse(buf, len); return ((Parser3 *)parser)->parse(buf, len);
} }
} }
src/test.cpp
+4
View File
@@ -330,6 +330,10 @@ TEST_CASE("parse rejects negative length") {
WeaselJsonParser_destroy(parser); WeaselJsonParser_destroy(parser);
} }
TEST_CASE("Calling parse with nullptr doesn't crash") {
REQUIRE(WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL);
}
andrew marked this conversation as resolved
weaselbot commented 2026-07-15 18:01:40 +00:00
Review
Copy Link
Copy Source

This couples the null-parser check with the negative-length case. Because the null check runs first it returns WeaselJson_NULL, but it would be clearer to use a valid length (e.g. WeaselJsonParser_parse(nullptr, buf, 5)) so the test isolates the null-parser behavior. It'd also be nice to mirror issue #51's exact reproduction - WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL - to document that buf=nullptr,len=0 is handled too.

This couples the null-parser check with the negative-length case. Because the null check runs first it returns WeaselJson_NULL, but it would be clearer to use a valid length (e.g. WeaselJsonParser_parse(nullptr, buf, 5)) so the test isolates the null-parser behavior. It'd also be nice to mirror issue #51's exact reproduction - WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL - to document that buf=nullptr,len=0 is handled too.
TEST_CASE("streaming") { testStreaming(json); } TEST_CASE("streaming") { testStreaming(json); }
TEST_CASE("reset clears inKey and transient state") { TEST_CASE("reset clears inKey and transient state") {
src/validate.cpp
+3
View File
@@ -33,6 +33,9 @@ int main(int argc, char **argv) {
case WeaselJson_REJECT: case WeaselJson_REJECT:
case WeaselJson_OVERFLOW: case WeaselJson_OVERFLOW:
return 1; return 1;
case WeaselJson_NULL:
fprintf(stderr, "parse called with a null parser\n");
andrew marked this conversation as resolved Outdated
weaselbot commented 2026-07-15 18:01:40 +00:00
Outdated
Review
Copy Link
Copy Source

This message ("Could not create parser") describes a different condition than the status: it's reached when parse() is called with a null pointer, not when create() failed. Something like fprintf(stderr, "parse called with a null parser\n"); would be clearer. (In this program the two coincide because parser.get() is only null when create failed, but the wording is still misleading relative to the status name.)

This message ("Could not create parser") describes a different condition than the status: it's reached when parse() is called with a null pointer, not when create() failed. Something like fprintf(stderr, "parse called with a null parser\n"); would be clearer. (In this program the two coincide because parser.get() is only null when create failed, but the wording is still misleading relative to the status name.)
return 1;
} }
if (l == 0) { if (l == 0) {
return 1; return 1;
weaseljson.py
+1
View File
@@ -30,6 +30,7 @@ class WeaselJsonStatus(enum.Enum):
AGAIN = 1 AGAIN = 1
REJECT = 2 REJECT = 2
OVERFLOW = 3 OVERFLOW = 3
andrew marked this conversation as resolved
weaselbot commented 2026-07-15 21:52:37 +00:00
Review
Copy Link
Copy Source

The C enum now has WeaselJson_NULL (value 4), but this Python enum stops at OVERFLOW = 3. It's never returned through the bindings today (parse() raises via _check_open before calling into C when the parser is null), but please add NULL = 4 here so the two stay in sync — otherwise a future change that lets the raw status through would surface an unmapped value.

The C enum now has WeaselJson_NULL (value 4), but this Python enum stops at OVERFLOW = 3. It's never returned through the bindings today (parse() raises via _check_open before calling into C when the parser is null), but please add `NULL = 4` here so the two stay in sync — otherwise a future change that lets the raw status through would surface an unmapped value.
NULL = 4
class WeaselJsonCallbacksBase: class WeaselJsonCallbacksBase: