-
Notifications
You must be signed in to change notification settings - Fork 25.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: check parameters in json-write.c #1576
base: master
Are you sure you want to change the base?
Conversation
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 <870355373@qq.com>
/submit |
Submitted as pull.1576.git.git.1695124498925.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
On the Git mailing list, Taylor Blau wrote (reply to this): [+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 |
User |
On the Git mailing list, Jeff Hostetler wrote (reply to this): 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 |
User |
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff Hostetler <git@jeffhostetler.com> 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 |
When I used the json-writer.c file as a lib, I found that it often caused coredump errors, so I submitted this patch
Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
Please read the "guidelines for contributing" linked above!
cc: Taylor Blau me@ttaylorr.com
cc: Jeff Hostetler git@jeffhostetler.com