From: Jeff Hostetler <git@jeffhostetler.com>
To: Taylor Blau <me@ttaylorr.com>,
mark via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, mark <870355373@qq.com>,
wangsirun <wangsirun@zhidaoauto.com>,
Jeff Hostetler <jeffhostetler@github.com>
Subject: Re: [PATCH] fix: check parameters in json-write.c
Date: Wed, 20 Sep 2023 16:02:00 -0400 [thread overview]
Message-ID: <dc45106c-d569-3438-d2ff-c3c94b6161d7@jeffhostetler.com> (raw)
In-Reply-To: <ZQne3ThSw6HVmNJc@nand.local>
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
next prev parent reply other threads:[~2023-09-20 20:02 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 11:54 [PATCH] fix: check parameters in json-write.c mark via GitGitGadget
2023-09-19 17:48 ` Taylor Blau
2023-09-20 20:02 ` Jeff Hostetler [this message]
2023-09-20 20:10 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc45106c-d569-3438-d2ff-c3c94b6161d7@jeffhostetler.com \
--to=git@jeffhostetler.com \
--cc=870355373@qq.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jeffhostetler@github.com \
--cc=me@ttaylorr.com \
--cc=wangsirun@zhidaoauto.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).