* [PATCH] fix: check parameters in json-write.c
@ 2023-09-19 11:54 mark via GitGitGadget
2023-09-19 17:48 ` Taylor Blau
0 siblings, 1 reply; 4+ messages in thread
From: mark via GitGitGadget @ 2023-09-19 11:54 UTC (permalink / raw)
To: git; +Cc: mark, wangsirun
From: wangsirun <wangsirun@zhidaoauto.com>
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>
---
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(+)
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 == '"')
base-commit: d4a83d07b8cc66d4afac2f33a8af729f2ba93bba
--
gitgitgadget
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fix: check parameters in json-write.c
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
0 siblings, 1 reply; 4+ messages in thread
From: Taylor Blau @ 2023-09-19 17:48 UTC (permalink / raw)
To: mark via GitGitGadget; +Cc: git, mark, wangsirun, Jeff Hostetler
[+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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix: check parameters in json-write.c
2023-09-19 17:48 ` Taylor Blau
@ 2023-09-20 20:02 ` Jeff Hostetler
2023-09-20 20:10 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Hostetler @ 2023-09-20 20:02 UTC (permalink / raw)
To: Taylor Blau, mark via GitGitGadget; +Cc: git, mark, wangsirun, Jeff Hostetler
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix: check parameters in json-write.c
2023-09-20 20:02 ` Jeff Hostetler
@ 2023-09-20 20:10 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2023-09-20 20:10 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Taylor Blau, mark via GitGitGadget, git, mark, wangsirun, Jeff Hostetler
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-20 20:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2023-09-20 20:10 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/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).