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
+13
4 changed files with 13 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 09c0fb72ca - Show all commits
include/weaseljson.h
+2
View File
@@ -38,6 +38,8 @@ enum WeaselJsonStatus {
WeaselJson_REJECT,
/** json is too deeply nested */
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;
1
src/lib.cpp
+3
View File
@@ -46,6 +46,9 @@ WeaselJsonParser_destroy(WeaselJsonParser *parser) {
__attribute__((visibility("default"))) WeaselJsonStatus
WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf, int len) {
if (parser == nullptr) [[unlikely]] {
return WeaselJson_NULL;
}
return ((Parser3 *)parser)->parse(buf, len);
}
}
src/test.cpp
+5
View File
@@ -330,6 +330,11 @@ TEST_CASE("parse rejects negative length") {
WeaselJsonParser_destroy(parser);
}
TEST_CASE("Calling parse with nullptr doesn't crash") {
char buf[10] = "hello";
REQUIRE(WeaselJsonParser_parse(nullptr, buf, -1) == 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("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_OVERFLOW:
return 1;
case WeaselJson_NULL:
fprintf(stderr, "Could not create 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) {
return 1;