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 */
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");
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: