{"thread":{"id":"60245","subject":"[PATCH] fix: check parameters in json-write.c","startedAt":"2023-09-19T11:55:06Z","lastAt":"2023-09-20T20:11:16Z","messageCount":4,"participants":["mark via GitGitGadget","Taylor Blau","Jeff Hostetler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482011","messageId":"pull.1576.git.git.1695124498925.gitgitgadget@gmail.com","threadId":"60245","inReplyTo":null,"subject":"[PATCH] fix: check parameters in json-write.c","fromName":"mark via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-19T11:54:58Z","receivedAt":"2023-09-19T11:55:06Z","isPatch":true,"sender":{"key":"markkicks@gmail.com","avatar":null},"body":"From: wangsirun \n\nWhen I used the json-writer.c file as a lib, I found that\nit often caused coredump errors, so I submitted this patch\n\nSigned-off-by: sirun Wang <870355373@qq.com>\n---\n fix: check parameters in json-write.c\n \n When I used the json-writer.c file as a lib, I found that it often\n caused coredump errors, so I submitted this patch\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1576%2Fwangsirun%2Fjson-write-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1576/wangsirun/json-write-fix-v1\nPull-Request: https://github.com/git/git/pull/1576\n\n json-writer.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/json-writer.c b/json-writer.c\nindex 005c820aa42..23ba7046e5d 100644\n--- a/json-writer.c\n+++ b/json-writer.c\n@@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)\n {\n \tunsigned char c;\n \n+\tif (!in || !*in) {\n+\t\tstrbuf_addstr(out, \"\\\"\\\"\");\n+\t\treturn;\n+\t}\n+\n \tstrbuf_addch(out, '\"');\n \twhile ((c = *in++) != '\\0') {\n \t\tif (c == '\"')\n\nbase-commit: d4a83d07b8cc66d4afac2f33a8af729f2ba93bba\n-- \ngitgitgadget\n"},{"id":"482019","messageId":"ZQne3ThSw6HVmNJc@nand.local","threadId":"60245","inReplyTo":"pull.1576.git.git.1695124498925.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix: check parameters in json-write.c","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-19T17:48:13Z","receivedAt":"2023-09-19T17:48:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[+cc Jeff Hostetler]\n\nOn Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:\n> diff --git a/json-writer.c b/json-writer.c\n> index 005c820aa42..23ba7046e5d 100644\n> --- a/json-writer.c\n> +++ b/json-writer.c\n> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)\n> {\n> \tunsigned char c;\n>\n> +\tif (!in || !*in) {\n> +\t\tstrbuf_addstr(out, \"\\\"\\\"\");\n> +\t\treturn;\n> +\t}\n\nFrom reading the implementation of append_quoted_string(), I think that\nthe case where \"in\" is the empty string is already covered. IOW, doing\nsomething like:\n\n struct strbuf buf = STRBUF_INIT;\n append_quoted_string(&out, \"\");\n warning(\"'%s'\", buf.buf);\n\nwould print out something like:\n\n warning: '\"\"'\n\nas expected. Handling a NULL \"in\" argument is new behavior, but I am not\nsure if it is appropriate to coerce a NULL input into the empty string.\nI've CC'd the author of this code, whose opinion I trust more than my\nown here.\n\nThanks,\nTaylor\n"},{"id":"482086","messageId":"dc45106c-d569-3438-d2ff-c3c94b6161d7@jeffhostetler.com","threadId":"60245","inReplyTo":"ZQne3ThSw6HVmNJc@nand.local","subject":"Re: [PATCH] fix: check parameters in json-write.c","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-09-20T20:02:00Z","receivedAt":"2023-09-20T20:02:11Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/19/23 1:48 PM, Taylor Blau wrote:\n> [+cc Jeff Hostetler]\n> \n> On Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:\n>> diff --git a/json-writer.c b/json-writer.c\n>> index 005c820aa42..23ba7046e5d 100644\n>> --- a/json-writer.c\n>> +++ b/json-writer.c\n>> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)\n>> {\n>> \tunsigned char c;\n>>\n>> +\tif (!in || !*in) {\n>> +\t\tstrbuf_addstr(out, \"\\\"\\\"\");\n>> +\t\treturn;\n>> +\t}\n> \n> From reading the implementation of append_quoted_string(), I think that\n> the case where \"in\" is the empty string is already covered. IOW, doing\n> something like:\n> \n> struct strbuf buf = STRBUF_INIT;\n> append_quoted_string(&out, \"\");\n> warning(\"'%s'\", buf.buf);\n> \n> would print out something like:\n> \n> warning: '\"\"'\n> \n> as expected. Handling a NULL \"in\" argument is new behavior, but I am not\n> sure if it is appropriate to coerce a NULL input into the empty string.\n> I've CC'd the author of this code, whose opinion I trust more than my\n> own here.\n> \n> Thanks,\n> Taylor\n\nThere are three callers of `append_quoted_string()` and it is static\nto the json-writer.c code.\n\nBasically, in a JSON object, we have 2 uses:\n\n {\n \"\" : \"\",\n \"\" : ,\n ...\n }\n\nAnd in a JSON array, we have the other:\n\n [\n \"\",\n ...\n ]\n\nI suppose it is OK for the 2 string-value cases to assume a NULL pointer\ncould be written as \"\" in the JSON output. Although, I kinda think a\nNULL pointer should call BUG() as we have in the various assert_*()\nroutines. It really is a kind of logic error in the caller.\n\nRegardless what we decide for the case, in the \ncase, the resulting JSON would not be valid. We need for the key to\nbe a non-empty string. For example { \"\" : 1 } is not valid JSON.\nSo the key case should call BUG() and not try to hide it.\n\nSo I'm leaning towards just making it a BUG() in all cases, but I'm\nopen to the other mixed handling.\n\nJeff\n"},{"id":"482087","messageId":"xmqqil848v1s.fsf@gitster.g","threadId":"60245","inReplyTo":"dc45106c-d569-3438-d2ff-c3c94b6161d7@jeffhostetler.com","subject":"Re: [PATCH] fix: check parameters in json-write.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T20:10:55Z","receivedAt":"2023-09-20T20:11:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> I suppose it is OK for the 2 string-value cases to assume a NULL pointer\n> could be written as \"\" in the JSON output. Although, I kinda think a\n> NULL pointer should call BUG() as we have in the various assert_*()\n> routines. It really is a kind of logic error in the caller.\n\nFWIW, that is my preference, too.\n\n> Regardless what we decide for the case, in the \n> case, the resulting JSON would not be valid. We need for the key to\n> be a non-empty string. For example { \"\" : 1 } is not valid JSON.\n> So the key case should call BUG() and not try to hide it.\n\nI do not have a strong opinion on this side, and leave it up to the\narea experts ;-)\n\n>\n> So I'm leaning towards just making it a BUG() in all cases, but I'm\n> open to the other mixed handling.\n>\n> Jeff\n"}]}