fix: check parameters in json-write.c

Commit Message

wangsirun Sept. 19, 2023, 11:54 a.m. UTC

From: wangsirun <[e-mail]>

When I used the json-writer.c file as a lib, I found that
it often caused coredump errors, so I submitted this patch

Signed-off-by: sirun Wang <[e-mail]>
---
    fix: check parameters in json-write.c
    
    When I used the json-writer.c file as a lib, I found that it often
    caused coredump errors, so I submitted this patch

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1576%2Fwangsirun%2Fjson-write-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1576/wangsirun/json-write-fix-v1
Pull-Request: https://github.com/git/git/pull/1576

 json-writer.c | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: d4a83d07b8cc66d4afac2f33a8af729f2ba93bba

Comments

Taylor Blau Sept. 19, 2023, 5:48 p.m. UTC | #1

[+cc Jeff Hostetler]

On Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:
> diff --git a/json-writer.c b/json-writer.c
> index 005c820aa42..23ba7046e5d 100644
> --- a/json-writer.c
> +++ b/json-writer.c
> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)
>  {
>  	unsigned char c;
>
> +	if (!in || !*in) {
> +		strbuf_addstr(out, "\"\"");
> +		return;
> +	}

From reading the implementation of append_quoted_string(), I think that
the case where "in" is the empty string is already covered. IOW, doing
something like:

    struct strbuf buf = STRBUF_INIT;
    append_quoted_string(&out, "");
    warning("'%s'", buf.buf);

would print out something like:

    warning: '""'

as expected. Handling a NULL "in" argument is new behavior, but I am not
sure if it is appropriate to coerce a NULL input into the empty string.
I've CC'd the author of this code, whose opinion I trust more than my
own here.

Thanks,
Taylor

Jeff Hostetler Sept. 20, 2023, 8:02 p.m. UTC | #2

On 9/19/23 1:48 PM, Taylor Blau wrote:
> [+cc Jeff Hostetler]
> 
> On Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:
>> diff --git a/json-writer.c b/json-writer.c
>> index 005c820aa42..23ba7046e5d 100644
>> --- a/json-writer.c
>> +++ b/json-writer.c
>> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)
>>   {
>>   	unsigned char c;
>>
>> +	if (!in || !*in) {
>> +		strbuf_addstr(out, "\"\"");
>> +		return;
>> +	}
> 
>  From reading the implementation of append_quoted_string(), I think that
> the case where "in" is the empty string is already covered. IOW, doing
> something like:
> 
>      struct strbuf buf = STRBUF_INIT;
>      append_quoted_string(&out, "");
>      warning("'%s'", buf.buf);
> 
> would print out something like:
> 
>      warning: '""'
> 
> as expected. Handling a NULL "in" argument is new behavior, but I am not
> sure if it is appropriate to coerce a NULL input into the empty string.
> I've CC'd the author of this code, whose opinion I trust more than my
> own here.
> 
> Thanks,
> Taylor

There are three callers of `append_quoted_string()` and it is static
to the json-writer.c code.

Basically, in a JSON object, we have 2 uses:

     {
         "<key>" : "<string-value>",
         "<key>" : <integer>,
         ...
     }

And in a JSON array, we have the other:

     [
         "<string-value>",
         ...
     ]

I suppose it is OK for the 2 string-value cases to assume a NULL pointer
could be written as "" in the JSON output.  Although, I kinda think a
NULL pointer should call BUG() as we have in the various assert_*()
routines. It really is a kind of logic error in the caller.

Regardless what we decide for the <string-value> case, in the <key>
case, the resulting JSON would not be valid. We need for the key to
be a non-empty string.  For example { "" : 1 } is not valid JSON.
So the key case should call BUG() and not try to hide it.

So I'm leaning towards just making it a BUG() in all cases, but I'm
open to the other mixed handling.

Jeff

Junio C Hamano Sept. 20, 2023, 8:10 p.m. UTC | #3

Jeff Hostetler <[e-mail]> writes:

> I suppose it is OK for the 2 string-value cases to assume a NULL pointer
> could be written as "" in the JSON output.  Although, I kinda think a
> NULL pointer should call BUG() as we have in the various assert_*()
> routines. It really is a kind of logic error in the caller.

FWIW, that is my preference, too.

> Regardless what we decide for the <string-value> case, in the <key>
> case, the resulting JSON would not be valid. We need for the key to
> be a non-empty string.  For example { "" : 1 } is not valid JSON.
> So the key case should call BUG() and not try to hide it.

I do not have a strong opinion on this side, and leave it up to the
area experts ;-)

>
> So I'm leaning towards just making it a BUG() in all cases, but I'm
> open to the other mixed handling.
>
> Jeff

Patch

diff --git a/json-writer.c b/json-writer.c
index 005c820aa42..23ba7046e5d 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -20,6 +20,11 @@  static void append_quoted_string(struct strbuf *out, const char *in)
 {
 	unsigned char c;
 
+	if (!in || !*in) {
+		strbuf_addstr(out, "\"\"");
+		return;
+	}
+
 	strbuf_addch(out, '"');
 	while ((c = *in++) != '\0') {
 		if (c == '"')